Closed glassfishrobot closed 10 years ago
glassfishrobot
commented
6 years ago glassfishrobot
commented
10 years ago @glassfishrobot Commented Reported by raghucbz
glassfishrobot
commented
10 years ago @glassfishrobot Commented raghucbz said: 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;
}@glassfishrobot Commented @pavelbucek said: thanks, thats ok, I can take it from here.
will keep this issue updated.
glassfishrobot
commented
10 years ago @glassfishrobot Commented @pavelbucek said: 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..
glassfishrobot
commented
10 years ago @glassfishrobot Commented raghucbz said: 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!
glassfishrobot
commented
10 years ago @glassfishrobot Commented @pavelbucek said: 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).
glassfishrobot
commented
10 years ago @glassfishrobot Commented raghucbz said: 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?
glassfishrobot
commented
10 years ago @glassfishrobot Commented @pavelbucek said: 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.
glassfishrobot
commented
10 years ago @glassfishrobot Commented @pavelbucek said: TCK passed without any issue, fix is now merged to the master branch.
glassfishrobot
commented
10 years ago @glassfishrobot Commented raghucbz said: Awesome, thanks! Appreciate your help.
glassfishrobot
commented
7 years ago @glassfishrobot Commented This issue was imported from java.net JIRA TYRUS-261
glassfishrobot
commented
10 years ago @glassfishrobot Commented Marked as fixed on Thursday, October 24th 2013, 8:43:04 am
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)
Code that throws the exception - EndpointWrapper.onMessage(SPIRemoteEndpoint gs, ByteBuffer messageBytes)
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.
Please let me know if you have further questions. Thanks! – Raghuram Krishnamachari aka raghu raghuramcbz at gmail dot com
Affected Versions
[1.2.1]