Fix possible NPE from WebSocketAdapter #7294
Conversation
Signed-off-by: Lachlan Roberts <[email protected]>
|
Do you want this in the 10.0.8 release? |
|
|
||
| public RemoteEndpoint getRemote() | ||
| { | ||
| return session.getRemote(); | ||
| return remote; |
There was a problem hiding this comment.
IMO return session == null ? null : session.getRemote(); might be better. This is minor, but could save the remote field. And I think the readability is not affected too much either way.
There was a problem hiding this comment.
It would be more surprising for getRemote() to suddenly start returning null after it was non-null.
Let the usage of Remote trigger the exceptions indicating that you no longer have a valid connection/session/whatnot.
| @@ -34,7 +35,8 @@ public Session getSession() | |||
|
|
|||
| public boolean isConnected() | |||
| { | |||
| return session.isOpen(); | |||
| Session sess = this.session; | |||
| return (sess != null) && (sess.isOpen()); | |||
There was a problem hiding this comment.
I think just return session != null && session.isOpen(); should be more readable. Creating sess here seems redundant.
There was a problem hiding this comment.
This is standard multi threading coding practice.
The value of this.session can actually change between the 2 evaluations (non-null check and isopen).
It could go from non-null to null between the two checks.
originally from PR #7293 by @azurvii
Fixes potential NPE from
WebSocketAdapterif used beforeonWebSocketConnect()is called.