Missing null checks in AbstractMessageChannel.addInterceptor/setInterceptors

[garyrussell]

Causes downstream NPEs at runtime...

https://stackoverflow.com/questions/61828635/occassional-nullpointerexception-when-publishing-stomp-message-using-springframe/61828913#61828913

Caused by: java.lang.NullPointerException
	at org.springframework.messaging.support.AbstractMessageChannel$ChannelInterceptorChain.applyPreSend(AbstractMessageChannel.java:178)
	at org.springframework.messaging.support.AbstractMessageChannel.send(AbstractMessageChannel.java:132)
	... 2 more

[jhoeller]

Gary, are you asking for assertions there? Or do you expect it to ignore null values in addInterceptor?

Generally speaking, we don't necessarily add assertions for non-null arguments at this point, relying on our nullability semantics instead. It doesn't hurt to assert for it nevertheless, in particular when adding elements to internal collections.

[jhoeller]

Looking around, we tend to have assertions in individual registration methods a la addInterceptor but don't usually have them in bulk methods such as setInterceptors (where otherwise we couldn't conveniently use addAll and co). That seems sensible since collection arguments are never really meant to contain null values anywhere in Spring, whereas individual setters sometimes accept null as an indication for resetting the value. From that perspective, I'm inclinded to add an assertion to addInterceptor here while leaving setInterceptors as-is.

[garyrussell]

relying on our nullability semantics instead.

Unfortunately, @Nullable annotation analysis is off by default in eclipse; I am not convinced that many developers enable it; I am not sure about IDEA.

(Not a problem with Kotlin, of course).

With collections (that can be nulled), we generally use Assert.noNullElements().

[jhoeller]

Good point, for List<HttpMessageConverter> in RestTemplate and co, we do assert no null elements as well. I'll go for the same in AbstractMessageChannel then and will also check a few other places.

[tangmin823]

@jhoeller If you don't add null checks, there will be null exceptions.