Hi Joe,
I didn't follow up previously on your close review (thankyou) of v002 of
the API - here's my thoughts and updates:
>
>
>
> ---- Minor API questions/suggestions/opinons ----
>
> *Endpoint.hasXXX()*: More of a stylistic thing, but elsewhere the
> onXXX convention is used - can we be consistent?
>
* yes lets, the naming was attempting to indicate more detail about when
the callback happens but...I agree onXX() is more typical. Changed for v003.
> *Endpoint.handleError()*: This seems a bit generic. At the very least,
> we should be explicit on whether this is for handling uncaught
> exception in user code (MessageListeners), or network related problems
> - which require very different treatment depending on the nature of
> the application.
good point. I did add the different categories of exception in the
javadoc for now, with a note that we may end up with something that is
less of a catch-all. Perhaps we should be feeding the MessageHandlers
their own errors back ? We may also want the implementation to keep
notifying MessageHandlers even if there is one bad MessageHandler that
keeps throwing runtime exceptions, rather than stop the message handling
for one bad handler.
>
> *Endpoint.handleError()*: Should this be able to handle Throwable (not
> just Exception)?
Yes I changed it to handle throwable, for now.
>
> *Session.addEncode()/Session.addMessageListener()*: Should we also
> supply equivalent remove methods? An example case for this is when an
> initial listener needs to replace itself with a more appropriate
> listener once it learns more about the client.
That does seem to be a useful use case, though it also raises the issue
of whether the ordering is significant. For now v3 models it as a Set
and I added the API access to modify it. Let's see what people think.
>
> *Session.setMaximumMessageSize()/Session.setTimeout()*: What are the
> defaults if these are not set (container specific)? Does the user have
> the facility to set inifinite values (i.e. never timeout)?
On idea would be to add defaults on the Endpoint configuration that
would apply to all sessions created from that configuration. And declare
System.properties to define the default value of the endpoint
configuration defaults ?
>
> *Session.isActive()/Session.isSecure()*: Danny, I think you messed up
> the source code here... isSecure() does not appear in the JavaDoc, and
> isActive() contains a bit of isSecure().
* whoops thanks for catching that. I should be fixed in v003.
>
> *Session.getHttpSession()*: HttpSessions are typically considered
> server side things - this makes little sense for a WebSocket client.
> Although we want to maintain as much API consistency as possible
> between server and client, we also need to cater for things that don't
> make sense in a both contexts.
>
Good point. Maybe we should explicitly factor that out into a subtype
that represents the Session-with-with-ref-to-HttpSession only in the web
container setting ? Right now it returns null if there is no HttpSession
available.
> *Session*: I can't imagine doing anything useful without the
> HttpRequest (allowing users to get things like user Principal, request
> parameters, HTTP headers, cookies, remote host, etc). At the same time
> I see the argument against (especially in a client, or non Servlety
> deployment). Maybe there should be sub-interfaces that provide more
> environment specific capabilities (e.g. ClientSession, ServerSession).
See what you think of the new handshake api on ServerConfiguration - I
think this separates out the differences a bit better between the client
and server in that regard.
>
> *Session*: Vocabulary thing... Session is such an overloaded term in
> the JEE space. How about something with a more meaningful description,
> like Connection?
Yeah, I agree Session is perhaps JavaEE centric.... 'Connection' is
better matched to the vocabulary of the web socket spec but seems to
have two disadvantages: the possibility of confusion with with
JDK-connection classes like URLConnection and friends, plus this API is
currently modeling something much higher level that the connection level
ideas in the websocket spec. 'Conversation' was another possibility. The
spec also talks about two-way-communication a lot, which is the right
level, but a bit literal minded.
>
> *RemoteEndpoint*: If the user is not using a custom encoder, what
> would type <T> be? Would they have to pass RemoteEndpoint<Void> around
> their code?
* I was thinking we could handle basic Java types for people, for
convenience - like all the primitive types and class equivalents...and
collections thereof. That kind of thing.
>
> *EncodeException/DecodeException*: Given that encoding is an IO
> related activity, should this extend IOException?
Well, it probably doesn't make much difference to developers either way,
but I think I'd say encoding decoding is its own activity: if I were at
the UN and the interpreter fainted, I wouldn't call it a failure of the
electronics in the headset :)
>
> *ServerConfiguration.checkOrigin()/getPreferredSubprotocol()*: It
> wasn't clear how to change the behavior here. Is ServerConfiguration
> meant to be subclassed? This seems a little messy as it inherits
> members which are clearly not meant to be overriden (e.g. getURI()).
Yes. I moved this to interfaces with default implementations to clarofy
for v3. See if its cleaner.
>
> *MessageListener*: This would be cleaner if it was generified with a
> void onMessage(T data) method, as opposed to an empty interface. It
> will avoid instanceof checks and casting elsewhere in infrastructure
> and cross-cutting services (e.g. generic MessageListener decorators).
> Ditto for Encode/Decoder.
OK, let me look at that for next time. On the face of it with the
streaming apis we now have, I don't know how we can have a catch-all
onMessage() method on the top level interface, but perhaps we can move
things around into a better arrangement.
>
> *MessageListener.onMessage()*: It would be convenient if this also
> provided a Session parameter, so a single MessageListener instance
> could be used to handle multiple connections.
We could, though I had envisioned that developers would use one
MessageListener/Handler instance for each session. Doing all the
handling in one place for all sessions could make the streaming methods
particularly awkward (state of a partial message from multiple sessions
to maintain, all in one method...).
>
> *Close.Code.TLS__HANDSHAKE_FAILURE*: Why the double underscore?
fixed.
Cheers,
- Danny
>
>
--
<http://www.oracle.com> *Danny Coward *
Java EE
Oracle Corporation