RemoteEndpoint.Async methods could provide better connection back to the Session

[glassfishrobot]

In with the callback and the Future version of the RemoteEndpoint.Async there is no obvious way to discover what the original Session object was, this makes things a little bit more complicated when you are trying to broadcast to a number of listening clients.

Ideally the "SendResult" object should have a reference to the original Session that way the SendMessage object could be implemented with once instance rather than one for each Session. This would allow the broadcasting code to be able to check the state of the Session, perhaps dealing with a odd exception by checking to see if the Session is still open / or valid.

Having to write some kind of if/else code to deal with the Throwable parameter on SendResult also feels a little bit dirty particular as normally we are expecting SessionsException. Could we always get a SessionException instead that might have a more generic cause - this would make life a bit easier. I guess this would also be another way to get hold of the Session consistently.

It would also be nice for consistency if the Future returned by the polling version of the API instead of returning Future<Void> return Future<SendResult> for consistency. Although obviously the semantics might be different in a failure case - I suppose it could also return Future<Session> but it would be less flexible.

Affected Versions

[1.0]

[glassfishrobot]

@glassfishrobot Commented
Reported by gdavison

[glassfishrobot]

@glassfishrobot Commented
This issue was imported from java.net JIRA WEBSOCKET_SPEC-185

[glassfishrobot]

• Issue Imported From: https://github.com/javaee/websocket-spec/issues/185
• Original Issue Raised By:@glassfishrobot
• Original Issue Assigned To: @pavelbucek

[markt-asf]

Exposing the session via the SendResult looks possible.

The comments re SessionException appear to be implementation specific. Tomcat, for example, never uses it. Neither does the WebSocket API. I wonder if we should deprecate it.

Exposing the Session via SendResult does mean we'd need to do something about exposing the same information via Future.

[joakime]

I'd like to come up with a solution that's lamda friendly 🤓

[markt-asf]

@joakime What did you have in mind over and above adding Session to SendResult and returning Future<SendResult> rather than Future<Void>?

Note: In the Future case, any errors will be exposed via a call to Future.get() throwing an ExecutionException so any SendResult returned via a Future is always going to be successful.

[joakime]

@markt-asf Having the SendResult.getSession() seems like a good choice at first blush.
Let me think about that approach.

So you see a use that might look like this?

RemoteEndpoint.Async async = session.getAsyncRemote();
async.sendText("Sequence 1", result -> {
    Session asyncSession = result.getSession();
    if(!result.isOK()) {
        asyncSession.close();
    }
});

[markt-asf]

So you see a use that might look like this?

Yes, something along those lines.

[markt-asf]

I've been looking at this some more. Future<Void> -> Future<SendResult> isn't binary compatible so I don't see an easy way to do that. I think that is OK though as it is still relatively simple for the developer to get/keep a reference to the session if they need one without that change.

Adding SendResult.getSession() is binary compatible and addresses the bigger issue that there is no way for the developer to obtain a reference to theSession from SendHandler.onResult(SendResult).

I'll give it a couple of days and, unless the discussion goes in another direction, create a PR to add this new method.