[TYRUS-261] Text decoder disables Binary decoder if both are configured for the same server endpoint; endpoint cannot receive binary messages anymore Created: 22/Oct/13  Updated: 24/Oct/13  Resolved: 24/Oct/13

Status: Resolved
Project: tyrus
Component/s: server
Affects Version/s: 1.2.1
Fix Version/s: 1.3

Type: Bug Priority: Major
Reporter: raghucbz Assignee: Pavel Bucek
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Description   

When two decoders, one Text and one Binary, are configured into a programmatic Endpoint the Text decoder disables the Binary decoder. Only text messages can be received by the endpoint. Binary messages received throws this exception java.lang.IllegalStateException: Binary messageHandler not found.

According to RFC 6455 both Text and Binary messages are supported by a websocket endpoint and they have different opcodes in the frames (%x1 for text and %x2 for binary) to be distinguishable by the server.
This is also clarified in the implementation of the two EndpointWrapper.onMessage methods, one for Text one for Binary.

The sample code to reproduce this error is here: https://github.com/raghucbz/websocket_bug1.git
The project is setup using IntelliJ and contains both the client and the server.

The Text Client (endpoint.StampingClientText.java) works fine. Here is the error you get when you run the Binary Client (endpoint.StampingClientBinaryStream.java)

[2013-10-22T10:26:02.279-0400] [glassfish 4.0] [SEVERE] [] [] [tid: _ThreadID=23 _ThreadName=Thread-4] [timeMillis: 1382451962279] [levelValue: 1000] [[
  java.lang.IllegalStateException: Binary messageHandler not found. 
Session: 'SessionImpl{uri=/websocket_bug1/StampingServerEndpoint, id='a4da753f-9a49-4593-ae6d-242ca9a4c464', endpoint=EndpointWrapper{endpointClass=class
endpoint.StampingServerEndpoint, endpoint=null, uri='/websocket_bug1/StampingServerEndpoint', contextPath='/websocket_bug1'}}'.
	at org.glassfish.tyrus.core.*EndpointWrapper.onMessage*(EndpointWrapper.java:469)
	at org.glassfish.tyrus.server.TyrusEndpoint.onMessage(TyrusEndpoint.java:180)
	at org.glassfish.tyrus.websockets.DefaultWebSocket.onMessage(DefaultWebSocket.java:148)
	at org.glassfish.tyrus.websockets.frametypes.BinaryFrameType.respond(BinaryFrameType.java:52)
	at org.glassfish.tyrus.websockets.DataFrame.respond(DataFrame.java:102)
	at org.glassfish.tyrus.servlet.TyrusHttpUpgradeHandler.onDataAvailable(TyrusHttpUpgradeHandler.java:113)
	at org.apache.catalina.connector.InputBuffer$ReadHandlerImpl.processDataAvailable(InputBuffer.java:488)
	at org.apache.catalina.connector.InputBuffer$ReadHandlerImpl.onDataAvailable(InputBuffer.java:453)
	at org.glassfish.grizzly.http.io.InputBuffer.append(InputBuffer.java:855)
	at org.glassfish.grizzly.http.server.HttpServerFilter.handleRead(HttpServerFilter.java:222)
	at org.glassfish.grizzly.filterchain.ExecutorResolver$9.execute(ExecutorResolver.java:119)
	at org.glassfish.grizzly.filterchain.DefaultFilterChain.executeFilter(DefaultFilterChain.java:288)
	at org.glassfish.grizzly.filterchain.DefaultFilterChain.executeChainPart(DefaultFilterChain.java:206)
	at org.glassfish.grizzly.filterchain.DefaultFilterChain.execute(DefaultFilterChain.java:136)
	at org.glassfish.grizzly.filterchain.DefaultFilterChain.process(DefaultFilterChain.java:114)
	at org.glassfish.grizzly.ProcessorExecutor.execute(ProcessorExecutor.java:77)
	at org.glassfish.grizzly.nio.transport.TCPNIOTransport.fireIOEvent(TCPNIOTransport.java:838)
	at org.glassfish.grizzly.strategies.AbstractIOStrategy.fireIOEvent(AbstractIOStrategy.java:113)
	at org.glassfish.grizzly.strategies.WorkerThreadIOStrategy.run0(WorkerThreadIOStrategy.java:115)
	at org.glassfish.grizzly.strategies.WorkerThreadIOStrategy.access$100(WorkerThreadIOStrategy.java:55)
	at org.glassfish.grizzly.strategies.WorkerThreadIOStrategy$WorkerThreadRunnable.run(WorkerThreadIOStrategy.java:135)
	at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.doWork(AbstractThreadPool.java:564)
	at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.run(AbstractThreadPool.java:544)
	at java.lang.Thread.run(Thread.java:724)]]

Code that throws the exception - EndpointWrapper.onMessage(SPIRemoteEndpoint gs, ByteBuffer messageBytes)

if (session.isWholeBinaryHandlerPresent()) {
    session.notifyMessageHandlers(messageBytes, findApplicableDecoders(session, messageBytes, false));
} else if (session.isPartialBinaryHandlerPresent()) {
    session.notifyMessageHandlers(messageBytes, true);
} else {
    throw new IllegalStateException(String.format("Binary messageHandler not found. Session: '%s'.", session));
}

session.isWholeBinaryHandlerPresent() is the same as MessageHandlerManager.binaryWholeHandlerPresent and both are false.

When a programmatic endpoint is first called the onOpen() is executed and the messages handlers are registered using the call session.addMessageHandler() which in turn calls MessageHandlerManager.addMessageHandler(). In the code below you can see the comment that point to the if/else code block that skips processing the binary handlers if a text handler is found. If the two if/elses are converted to two if/ifs the problem would be solved.

public void addMessageHandler(MessageHandler handler) throws IllegalStateException {

    if (!(handler instanceof MessageHandler.Whole) && !(handler instanceof MessageHandler.Partial)) {
        throwException("MessageHandler must implement MessageHandler.Whole or MessageHandler.Partial.");
    }

    final Class<?> handlerClass = getHandlerType(handler);

    if (handler instanceof MessageHandler.Whole) { //WHOLE MESSAGE HANDLER
        if (WHOLE_TEXT_HANDLER_TYPES.contains(handlerClass)) { // text
            if (textHandlerPresent) {
                throwException("Text MessageHandler already registered.");
            } else {
                if (Reader.class.isAssignableFrom(handlerClass)) {
                    readerHandlerPresent = true;
                }
                textHandlerPresent = true;
                textWholeHandlerPresent = true;
            }
        } else if (WHOLE_BINARY_HANDLER_TYPES.contains(handlerClass)) { // binary
            if (binaryHandlerPresent) {
                throwException("Binary MessageHandler already registered.");
            } else {
                if (InputStream.class.isAssignableFrom(handlerClass)) {
                    inputStreamHandlerPresent = true;
                }
                binaryHandlerPresent = true;
                binaryWholeHandlerPresent = true;
            }
        } else if (PONG_HANDLER_TYPE == handlerClass) { // pong
            if (pongHandlerPresent) {
                throwException("Pong MessageHander already registered.");
            } else {
                pongHandlerPresent = true;
            }
        } else {
            boolean decoderExists = false;

            //IF/ELSE BLOCK THAT CHECKS FOR TEXT DECODERS AND IF FOUND SKIPS CHECK FOR BINARY DECODERS

            if (checkTextDecoders(handlerClass)) {//decodable text
                if (textHandlerPresent) {
                    throwException("Text MessageHandler already registered.");
                } else {
                    textHandlerPresent = true;
                    textWholeHandlerPresent = true;
                    decoderExists = true;
                }
            }
            else if (checkBinaryDecoders(handlerClass)) {//decodable binary
                if (binaryHandlerPresent) {
                    throwException("Text MessageHandler already registered.");
                } else {
                    binaryHandlerPresent = true;
                    binaryWholeHandlerPresent = true;
                    decoderExists = true;
                }
            }

            if (!decoderExists) {
                throwException(String.format("Decoder for type: %s has not been registered.", handlerClass));
            }
        }
    } else { // PARTIAL MESSAGE HANDLER

        //IF/ELSE BLOCK THAT CHECKS FOR TEXT DECODERS AND IF FOUND SKIPS CHECK FOR BINARY DECODERS

        if (PARTIAL_TEXT_HANDLER_TYPE.equals(handlerClass)) { // text
            if (textHandlerPresent) {
                throwException("Text MessageHandler already registered.");
            } else {
                textHandlerPresent = true;
            }
        }
        else if (PARTIAL_BINARY_HANDLER_TYPES.contains(handlerClass)) { // binary
            if (binaryHandlerPresent) {
                throwException("Binary MessageHandler already registered.");
            } else {
                binaryHandlerPresent = true;
            }
        } else {
            throwException(String.format("Partial MessageHandler can't be of type: %s.", handlerClass));
        }
    }

    // map of all registered handlers
    if (registeredHandlers.containsKey(handlerClass)) {
        throwException(String.format("MessageHandler for type: %s already registered.", handlerClass));
    } else {
        registeredHandlers.put(handlerClass, handler);
    }

    messageHandlerCache = null;
}

Please let me know if you have further questions.
Thanks!

Raghuram Krishnamachari aka raghu
raghuramcbz at gmail dot com



 Comments   
Comment by raghucbz [ 22/Oct/13 ]

I can fix this issue if someone gives me permission to work in this.

I fixed it in my environment (just removed the two else clause), built and replaced tyrus-core.jar in glassfish/modules. Glassfish server starts and both Text and clients work.

Let me know,
Thanks!

Raghu

Fixed method here:

public void addMessageHandler(MessageHandler handler) throws IllegalStateException {

    if (!(handler instanceof MessageHandler.Whole) && !(handler instanceof MessageHandler.Partial)) {
        throwException("MessageHandler must implement MessageHandler.Whole or MessageHandler.Partial.");
    }

    final Class<?> handlerClass = getHandlerType(handler);

    if (handler instanceof MessageHandler.Whole) { //WHOLE MESSAGE HANDLER
        if (WHOLE_TEXT_HANDLER_TYPES.contains(handlerClass)) { // text
            if (textHandlerPresent) {
                throwException("Text MessageHandler already registered.");
            } else {
                if (Reader.class.isAssignableFrom(handlerClass)) {
                    readerHandlerPresent = true;
                }
                textHandlerPresent = true;
                textWholeHandlerPresent = true;
            }
        } else if (WHOLE_BINARY_HANDLER_TYPES.contains(handlerClass)) { // binary
            if (binaryHandlerPresent) {
                throwException("Binary MessageHandler already registered.");
            } else {
                if (InputStream.class.isAssignableFrom(handlerClass)) {
                    inputStreamHandlerPresent = true;
                }
                binaryHandlerPresent = true;
                binaryWholeHandlerPresent = true;
            }
        } else if (PONG_HANDLER_TYPE == handlerClass) { // pong
            if (pongHandlerPresent) {
                throwException("Pong MessageHander already registered.");
            } else {
                pongHandlerPresent = true;
            }
        } else {
            boolean decoderExists = false;

            if (checkTextDecoders(handlerClass)) {//decodable text
                if (textHandlerPresent) {
                    throwException("Text MessageHandler already registered.");
                } else {
                    textHandlerPresent = true;
                    textWholeHandlerPresent = true;
                    decoderExists = true;
                }
            }
            if (checkBinaryDecoders(handlerClass)) {//decodable binary
                if (binaryHandlerPresent) {
                    throwException("Text MessageHandler already registered.");
                } else {
                    binaryHandlerPresent = true;
                    binaryWholeHandlerPresent = true;
                    decoderExists = true;
                }
            }

            if (!decoderExists) {
                throwException(String.format("Decoder for type: %s has not been registered.", handlerClass));
            }
        }
    } else { // PARTIAL MESSAGE HANDLER
        if (PARTIAL_TEXT_HANDLER_TYPE.equals(handlerClass)) { // text
            if (textHandlerPresent) {
                throwException("Text MessageHandler already registered.");
            } else {
                textHandlerPresent = true;
            }
        }
        if (PARTIAL_BINARY_HANDLER_TYPES.contains(handlerClass)) { // binary
            if (binaryHandlerPresent) {
                throwException("Binary MessageHandler already registered.");
            } else {
                binaryHandlerPresent = true;
            }
        } else {
            throwException(String.format("Partial MessageHandler can't be of type: %s.", handlerClass));
        }
    }

    // map of all registered handlers
    if (registeredHandlers.containsKey(handlerClass)) {
        throwException(String.format("MessageHandler for type: %s already registered.", handlerClass));
    } else {
        registeredHandlers.put(handlerClass, handler);
    }

    messageHandlerCache = null;
}
Comment by Pavel Bucek [ 22/Oct/13 ]

thanks, thats ok, I can take it from here.

will keep this issue updated.

Comment by Pavel Bucek [ 23/Oct/13 ]

Issue here is that you are combining use of two decoders on top of one message handler. This is definitely not intended use - if you create one message handler for text and another one for binary, it works as expected.

I will need to consult the specification more thoroughly to confirm whether this should be possible or not.

Anyway, thanks again for interesting issue..

Comment by raghucbz [ 23/Oct/13 ]

These are encoders/decoders for sendObject and we can't have multiple message handlers for them in the same endpoint. The test I have attached demonstrates this. We have multiple clients communicating with the server endpoint sending massive objects back and forth. If the client endpoint supports binary (like Java) we use binary encoder/decoder to serialize the object more effectively. If the client endpoint supports only text (like javascript) we use a text encoder/decoder to serialize the object using JSON. Now the server does not care what transport layer the client is using, all it cares about is implementing a message handler for that object. I know you are worried about a low level change, please let me know your concerns and we can discuss this more. I really think this is a wonderful feature of websockets that has to be put in use the right way.

Thanks!

Comment by Pavel Bucek [ 23/Oct/13 ]

its not just about being worried - change you are proposing breaks Tyrus unit tests (which was kind of expected) and it might possibly break TCK as well (which is not acceptable).

btw, from what I saw in Tyrus code and during my evaluation - it is possible and should be fine to have two decoders and two decodable message handlers, one for text and other one for binary messages, so this issue is definitely workaroundable (I can provide my testcase if you want).

Comment by raghucbz [ 23/Oct/13 ]

Ah I see, could it be this?
I found a possible bug in my fix above, it is in the Partial Message Handler. We need to introduce a decoderExists boolean, what do you think?

    } else { // PARTIAL MESSAGE HANDLER
        boolean decoderExists = false;

        if (PARTIAL_TEXT_HANDLER_TYPE.equals(handlerClass)) { // text
            if (textHandlerPresent) {
                throwException("Text MessageHandler already registered.");
            } else {
                textHandlerPresent = true;
                decoderExists = true;
            }
        }
        if (PARTIAL_BINARY_HANDLER_TYPES.contains(handlerClass)) { // binary
            if (binaryHandlerPresent) {
                throwException("Binary MessageHandler already registered.");
            } else {
                binaryHandlerPresent = true;
                decoderExists = true;
            }
        }

        if (!decoderExists) {
            throwException(String.format("Partial MessageHandler can't be of type: %s.", handlerClass));
        }
    }

Can you tell me how to have two decodable whole message handlers for the same object within the same endpoint? This is my sample endpoint below.

    public void onOpen(final Session session, EndpointConfig endpointConfig) {
        session.addMessageHandler(new MessageHandler.Whole<SimpleBean>() {

            @Override
            public void onMessage(SimpleBean bean) {
                String content = bean.getContent();
                String timestamp = new Date(System.currentTimeMillis()).toString();
                System.out.println("received content: " + content + " at " + timestamp);                
            }
        });
    }

Are the tests that are failing using negative testing to disallow two decoders for one message handler? Yea can you please provide test cases for workaround if possible?

Comment by Pavel Bucek [ 24/Oct/13 ]

ad 1) I have very similar solution in my local branch, we'll see whether that would be good enough for TCK and other tests.

ad 2)

workaround was meant to have two data types, one for binary, other one for text representation.. then, in each of these you could just call your method which could take super type of both.. something like:


    public static class TextAndBinaryDecoderEndpoint extends Endpoint {

        @Override
        public void onOpen(Session session, EndpointConfig config) {
            session.addMessageHandler(new MessageHandler.Whole<TextMessage>() {
                @Override
                public void onMessage(TextMessage message) {
                    handle(message);
                }
            });

            session.addMessageHandler(new MessageHandler.Whole<BinaryMessage>() {
                @Override
                public void onMessage(BinaryMessage message) {
                    handle(message);
                }
            });

        }

        public void handle(Message message) {
            System.out.println(message.toString());
        }
    }

    public static class Message {
    }

    public static class BinaryMessage extends Message {
    }

    public static class TextMessage extends Message {
    }

    public static class BinaryContainerDecoder extends CoderAdapter implements Decoder.Binary<BinaryMessage> {
        @Override
        public BinaryMessage decode(ByteBuffer bytes) throws DecodeException {
            return new BinaryMessage();
        }

        @Override
        public boolean willDecode(ByteBuffer bytes) {
            return true;
        }
    }

    public static class TextContainerDecoder extends CoderAdapter implements Decoder.Text<TextMessage> {
        @Override
        public TextMessage decode(String s) throws DecodeException {
            return new TextMessage();
        }

        @Override
        public boolean willDecode(String s) {
            return true;
        }
    }

it is not exactly what you want, it is slightly more complicated, but it achieves same thing - thats why I call it workaround.

Comment by Pavel Bucek [ 24/Oct/13 ]

TCK passed without any issue, fix is now merged to the master branch.

Comment by raghucbz [ 24/Oct/13 ]

Awesome, thanks! Appreciate your help.

Generated at Mon Feb 08 00:24:16 UTC 2016 using JIRA 6.2.3#6260-sha1:63ef1d6dac3f4f4d7db4c1effd405ba38ccdc558.