Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect @ServerEndpoint Encoder/Decoder lifecycle #425

Closed
brettwooldridge opened this issue Mar 14, 2016 · 1 comment
Closed

Incorrect @ServerEndpoint Encoder/Decoder lifecycle #425

brettwooldridge opened this issue Mar 14, 2016 · 1 comment
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@brettwooldridge
Copy link

I have a server endpoint defined as follows:

@ServerEndpoint(value = "/some/path/{token}",
                subprotocols = { "fst", "netty" },
                configurator = ServerEventConfigurator.class,
                decoders = SubprotoDecoderDelegator.class,
                encoders = SubprotoEncoderDelegator.class)
public class ServerEventEndpoint {
   ...
   @OnMessage
   public void onMessage(Session session, Event event) {
      eventBroker.dispatch(event);
   }
}

The ServerEventConfigurator configurator sets the negotiated protocol as a user property (hackish because of #424):

public class ServerEventConfigurator extends Configurator {
   @Override
   public void modifyHandshake(ServerEndpointConfig config, HandshakeRequest request, HandshakeResponse response) {
      List<String> requested = ((JsrHandshakeRequest) request).getHeaders().get(HandshakeRequest.SEC_WEBSOCKET_PROTOCOL);

      String subprotocol = super.getNegotiatedSubprotocol(config.getSubprotocols(), requested);
      config.getUserProperties().put("subprotocol", subprotocol);
   }
}

Finally, the decoder (SubprotoDecoderDelegator) with line numbers for commentary below:

01 public class SubprotoDecoderDelegator implements Binary<Event> {
02    private Binary<Event> delegate;
03 
04    public SubprotoDecoderDelegator() {
05       System.out.println("Constructed");
06    }
07 
08    @Override public void init(EndpointConfig config) {
09       String subprotocol = config.getUserProperties().get("subprotocol").toString();
10       switch (subprotocol) {
11          case "fst":
12             delegate = new FstEventDecoder();
13             break;
14          ...
15       }
16    }
17
18    @Override public Event decode(ByteBuffer bytes) throws DecodeException {
19       return delegate.decode(bytes);
20    }
21 }

When a client connects with sub-protocol "fst", we see that the configurator is hit, and correctly sets "fst" as a "subprotocol" property on the user properties.

Subsequently, a breakpoint on line 5 shows that the decoder is constructed from DecoderFactory, and that init(EndpointConfig) is called whereby the subprotocol is obtained (fst) and a delegate decoder is constructed (line 12). Everything appears fine at this point.

However, next the breakpoint on line 5 is hit again, this time from OnMessageCallable. For this new instance init(EndpointConfig) is never called. Subsequently, our breakpoint at line 19 is hit, invoked against this new instance that has no delegate set, resulting in an NPE.

The JavaDoc for javax.websocket.Decoder states:

/* The websocket implementation creates a new instance of the decoder per endpoint instance
 * per connection.  The lifecycle of the Decoder instance is governed by the container calls to
 * the Decoder.init(javax.websocket.EndpointConfig) and Decoder.destroy() methods.

Jetty does appear to be constructing a new instance "per endpoint instance" and "per connection", however, this instance is never used AFAICT. Instead, a new instance is being constructed, and its lifecycle methods never called, resulting in an NPE.

It seems clear that because the init() method accepts an EndpointConfig, from which user properties are available, the intention is that each En/Decoder instance should share the same "session" as was available in the Configurator.modifyHandshake(ServerEndpointConfig config) invocation.

@joakime joakime self-assigned this Mar 14, 2016
@joakime joakime added the Bug For general bugs on Jetty side label Mar 14, 2016
joakime added a commit that referenced this issue Jun 21, 2016
+ Duplicate Decoder was created and used in OnMessageCallable.
  Now using DecoderFactory to obtain already instantiated
  Decoder instead.
@joakime
Copy link
Contributor

joakime commented Jun 21, 2016

Duplicate Decoder was created and used in OnMessageCallable.
Now using DecoderFactory to obtain already instantiated and initialized Decoder instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

No branches or pull requests

2 participants