Thanks for looking these over - couple things inline...
On 1/19/13 8:39 AM, Justin Lee wrote:
We are using classes for various developer-provided things in the API - especially in the annotations - configuration on @WebSocketEndpoint, and of course the Encoder and Decoder classes.
On Fri, Jan 18, 2013 at 8:23 PM, Danny Coward < " title="[GMCP] Compose a new mail to " rel="noreferrer" target="_blank"> > wrote:
We have many issues to resolve, so I picked a collection to get things started. As usual, I have outlined an approach for each issue to keep things moving, so if you have other options or suggestions to add, please chime in.
* Consider requiring authentication schemes to be supported on the client container
I think BASIC and DIGEST are probably the most common. I think this is a good idea, but I am wary of trying to come up with this in a short period of time for the first version. Given that developers can intercept and modify the handshake request (see below) and inspect the handshake response, they have the ability to build http-based authentication schemes on top of the API without any changes. So I'd suggest we defer requiring client containers to support any particular authentication schemes until the next release.
Agreed. The hybi list spents weeks on this without any meaningful resolution that I could see. No need to rush it when existing solutions will work.
* Allow annotated client endpoints to intercept handshake (as well as programmatic ones)
Currently we have a scheme whereby programmatic client endpoint can override the default configuration (for example, to intercept the opening handshake). But there isn't a way to allow annotated client endpoints to do the same thing. Since server endpoints of both kinds can do this, and given developers may want to plug in authentication schemes or other header-manipulating code into client POJOs, I think we should allow this by (based on the current scheme) adding an optional configuration() attribute to @WebSocketClient.
Would it be a class parameter passed to the annotation? My fear about passing in class references and having the framework instantiate them is that it precludes the use of injection frameworks like guice/CDI to wire up those dependencies without some gymnastics inside the class's initializers to find the injection context and force that wiring explicitly.
When you talk about use of injection frameworks, I think what we allow for is the websocket implementation to delegate instance creation to other frameworks. For example, in the EE setting, to delegate endpoint instance creation to CDI so that EE things can be injected into endpoints. Is this what you had in mind ?
But using classes for a variety of things: configuration/encoders/decoders/applicationconfig we do preclude the application developer from creating instances of those things themselves, using their own favorite framework. I don't know how we'd do that, since we can't reference instances of things from annotations.For for this case, my thinking was that once the Session is closed developers really should not even be still using it. The same session can't be reopened, and the container probably wants to recycle it or forget it. So I was looking for an appropriate runtime exception. If we used a checked exception, it would be saying its reasonable to write code that calls sessions after they are invalid, and that developers always have to code with an error handing block in case. Whereas, they just shouldn't even get here.
* Allow Java primitives and class equivalents as message parameters for @WebSocketMessage methods.
Currently, you are allowed to annotate methods that offer a String, ByteBuffer, byte or custom developer object (with decoder) parameter for whole message processing or (String,boolean), (ByteBuffer, boolean) and (byte, boolean) parameters for partial message processing. The request is to allow methods that offer Java primitives for message processing.
Since all of the classes Byte, Short, Integer, Long, Float, Double, Boolean have unambiguous mappings from String via their single argument String constructors, I think this would be a nice addition to allow this for processing text messages. Character has one string parameter constructor, but of course String.toCharArray() is unambiguous enough for strings of length 1, so that could be included for completeness. Any failure to decode an incoming message to its primitive/class equivalent, according to those mapping would generate a DecodeException, in exactly the same way as happens for incoming messages that the runtime is trying to map to a custom developer object.
I don't see an unambiguous way to map binary data to Java primitive types, so I'm suggesting we make this work only for text messages.
* Lifecycle of Decoders/Encoders
Currently the specification leaves it open to implementations as to when they instantiate Encoders and Decoders for incoming and outgoing messages. We could leave this up to the implementations, but it would be better to define it since they are supposed to be portable. There are clearly a number of choices here, but I do think we should guarantee that no single instance of an Encoder or Decoder object is shared between different connections. What are other people's thoughts ?
See my comments above about passing classes around. I'm ok making that requirement clear in the docs.
* Calling methods on closed Sessions / Making Session Closeable
Once a websocket session has closed, it is possible that code will still reference the instance and (inadvertently?) call methods on it. In such situations, I think the methods should throw IllegalStateException because the session is closed and its data and operations are now meaningless.
The JDK throws IOException in that case (or some subclass). We might consider following that example for consistency's sake.
IllegalStateException seemed to fit that bill (http://docs.oracle.com/javase/7/docs/api/java/lang/IllegalStateException.html), and is also consistent with what HttpSession does after its been invalidated.Yes, and I think this is one of those things that 'shows well' too !
Secondly, since Session already has a close() method, we could make it implement java.io.Closable, and hence it would then be possible to use it in a JDK 7 try-with-resources block. I think this is a good idea too.
* Extension parameter ordering
Currently the API models extension parameters as Map<String, String>, yet the parameters have an order in the http headers. While this order doesn't have a meaning in the websocket protocol specification, both the MUX and Compression extensions attach semantic meaning to specific parameters, so it is perfectly possible that some new extension might attach semantic meaning to their order. So, I think our API needs to preserve them. I suggest we model extension parameters as List<Parameter> where Parameter is a member type of Extension, and contains the parameter name and value as Strings.
I don't think header ordering matters anywhere in any common protocols. I don't think we should bend over backwards making this a requirement outside of something forcing us to. I'd be really, really surprised if any extensions make it out of specification with that requirement. Even HTTP only specifies the first 2ish headings and that's merely to aid in protocol identification. I'm a -1 on this one.
* Number of instances of client container / server container
Currently these are both modeled using the class WebSocketContainer. This relates to two requests:
1) To allow the client developer to instantiate client container instances at will, so that he can use different instances with different property values, such as async send timeout or maximum message sizes. This seems like a reasonable thing to accommodate.
Absolutely. Consider a proxying websocket application trying to talk to two different servers for whatever reason.
2) To use a one instance of the server container WebSocketContainer per application per VM instead of the current one instance per VM for all applications. I think this is reasonable too - exactly matches the cardinality of the ServletContext for web components.
[jsr356-experts] Re: [jsr356-users] Re: Pot Pourri of issues from Public Draft