Intended behavior of ILog if "null" ILogListener is added

[ptziegler]

While manually executing some of our tests, I frequently saw the following exception in the logs:

java.lang.NullPointerException: Cannot invoke "org.eclipse.core.runtime.ILogListener.logging(org.eclipse.core.runtime.IStatus, String)" because "this.val$listener" is null
	at org.eclipse.core.internal.runtime.Log$1.run(Log.java:114)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:47)
	at org.eclipse.core.internal.runtime.Log.logToListeners(Log.java:122)
	at org.eclipse.core.internal.runtime.Log.logged(Log.java:101)
	at org.eclipse.osgi.internal.log.ExtendedLogReaderServiceFactory.safeLogged(ExtendedLogReaderServiceFactory.java:108)
	at org.eclipse.osgi.internal.log.ExtendedLogReaderServiceFactory.logPrivileged(ExtendedLogReaderServiceFactory.java:259)
	at org.eclipse.osgi.internal.log.ExtendedLogReaderServiceFactory.log(ExtendedLogReaderServiceFactory.java:229)
	at org.eclipse.osgi.internal.log.ExtendedLogServiceFactory.log(ExtendedLogServiceFactory.java:124)
	at org.eclipse.osgi.internal.log.LoggerImpl.log(LoggerImpl.java:93)
	at org.eclipse.osgi.internal.log.LoggerImpl.log(LoggerImpl.java:84)
	at org.eclipse.osgi.internal.log.LoggerImpl.log(LoggerImpl.java:79)
	at org.eclipse.core.internal.runtime.Log.log(Log.java:83)
	at org.eclipse.wb.internal.core.DesignerPlugin.log(DesignerPlugin.java:278)

Which is correct, of course, because I accidentally register a null ILogListener, which is then being notified. But it doesn'
t makes much sense to accept null values when calling addLogListener() if the implementation doesn't support it.

The JavaDoc sadly doesn't say anything about null listeners, so what would be the correct approach here? Ignore null listeners when they are being added? Throw a NullPointerException in addLogListener()? Or perhaps something else?

Here the relevant code sections:

eclipse.platform/runtime/bundles/org.eclipse.core.runtime/src/org/eclipse/core/internal/runtime/Log.java

Lines 51 to 55 in 5ccd447

	public void addLogListener(ILogListener listener) {
	synchronized (logListeners) {
	logListeners.add(listener);
	}
	}

eclipse.platform/runtime/bundles/org.eclipse.core.runtime/src/org/eclipse/core/internal/runtime/Log.java

Lines 104 to 124 in 5ccd447

	private void logToListeners(final IStatus status) {
	// create array to avoid concurrent access
	ILogListener[] listeners;
	synchronized (logListeners) {
	listeners = logListeners.toArray(new ILogListener[logListeners.size()]);
	}
	for (final ILogListener listener : listeners) {
	ISafeRunnable code = new ISafeRunnable() {
	@Override
	public void run() throws Exception {
	listener.logging(status, bundle.getSymbolicName());
	}
	@Override
	public void handleException(Throwable e) {
	//Ignore
	}
	};
	SafeRunner.run(code);
	}
	}

[fedejeanne]

What would be the benefit of accepting (and silently ignoring) null listeners in addLogListener(...)?

I would let them fail with a NPE but I don't know how many use-cases that would break. That would of course require to sharpen the JavaDocs and would IMO be a breaking change in the contract. Luckily, the Log class is inside internal so the change can be made without the need to deprecate the current method first.

[vogella]

Whoever registers null here deserves to receive an exception:-)

[ptziegler]

What would be the benefit of accepting (and silently ignoring) null listeners in addLogListener(...)?

It would prevent regressions on user-side if they happen to add null listeners. I don't see any additional benefit.

Luckily, the Log class is inside internal so the change can be made without the need to deprecate the current method first.

I don't follow... The method is defined by the ILog interface and therefore public API.

eclipse.platform/runtime/bundles/org.eclipse.core.runtime/src/org/eclipse/core/runtime/ILog.java

Lines 33 to 41 in 7c138cf

	/**
	* Adds the given log listener to this log. Subsequently the log listener will
	* receive notification of all log events passing through this log.
	* This method has no effect if the identical listener is already registered on this log.
	*
	* @param listener the listener to add to this log
	* @see Platform#addLogListener(ILogListener)
	*/
	public void addLogListener(ILogListener listener);

Whoever registers null here deserves to receive an exception:-)

That would be my expectation as well. You'd get an exception one way or the other (when you add a null listener or when you actually log something). Personally, I prefer to get the exception as early as possible, so that I don't have to search for the line of code where this invalid listener was added.

[fedejeanne]

Luckily, the Log class is inside internal so the change can be made without the need to deprecate the current method first.

I don't follow... The method is defined by the ILog interface and therefore public API.

eclipse.platform/runtime/bundles/org.eclipse.core.runtime/src/org/eclipse/core/runtime/ILog.java

Lines 33 to 41 in 7c138cf

	/**
	* Adds the given log listener to this log. Subsequently the log listener will
	* receive notification of all log events passing through this log.
	* This method has no effect if the identical listener is already registered on this log.
	*
	* @param listener the listener to add to this log
	* @see Platform#addLogListener(ILogListener)
	*/
	public void addLogListener(ILogListener listener);

You're right, I was looking at the implementation and not the interface. It is therefore technically a breaking API change, even if in reality it already throws a NPE when trying to log something. Since that is the case, I suggest to do it the same way Java does it for its collections e.g. for the Set interface, which leaves the decision of whether or not to accept null elements to the specific implementations:

The stipulation above does not imply that sets must accept all elements; sets may refuse to add any particular element, including null, and throw an exception, as described in the specification for Collection.add. Individual set implementations should clearly document any restrictions on the elements that they may contain.

btw. the same happens in other methods (I found them by following the JavaDoc/implementations):

• org.eclipse.core.runtime.ILog.addLogListener(ILogListener) mentions (in its JavaDoc)...
• org.eclipse.core.runtime.Platform.addLogListener(ILogListener) which calls...
• org.eclipse.core.internal.runtime.InternalPlatform.addLogListener(ILogListener) which calls...
• org.eclipse.core.internal.runtime.RuntimeLog.addLogListener(ILogListener)

[fedejeanne]

To be perfectly clear, what I suggest is:

• Change the JavaDocs and explicitly mention that the decision of whether or not to accept nulls is left to the implementation
• Throw a NPE in the implementation(s)

Since the behavior was not clearly specified before, I don't know if this is technically a breaking API change (@vogella ?), but I would still postpone the change until the next release cycle.

[vogella]

That would be my expectation as well. You'd get an exception one way or the other (when you add a null listener or when you actually log something).

Of course. If you concerned about potential NPE, wait with the merge of such a change after we open up for the next release, this gives users a changes to break up this over a full release cycle.