Handling of "glued" messages #286

Closed
kirach opened this Issue Apr 16, 2012 · 13 comments

Comments

Projects
None yet
4 participants

kirach commented Apr 16, 2012

Sometimes, while server sends asynchronous messages through broadcaster fast enough, on client side we can receive several messages "glued" in one message. I noticed it, using "streaming" transport. For example:

    //i'm using some groovy syntax sugar here, just for clear presentation
    if (request.getMethod().equalsIgnoreCase("GET")) {
            // First, suspend our request
            resource.suspend()
            // Second, broadcast several messages to clients.
        } else if (request.getMethod().equalsIgnoreCase("POST")) {
            def broadcaster = resource.getBroadcaster()
            broadcaster.broadcast(new JSONObject([command: "atmosphere/test", params: [testParam: "test"]]))
            broadcaster.broadcast(new JSONObject([command: "atmosphere/test", params: [testParam: "test2"]]))
            broadcaster.broadcast(new JSONObject([command: "atmosphere/test", params: [testParam: "test3"]]))
        }

In this case, I can get on client side message like this:

{"command":"atmosphere/test","params":{"testParam":"test"}}{"command":"atmosphere/test","params":{"testParam":"test2"}}{"command":"atmosphere/test","params":{"testParam":"test3"}}

Of course, I can manually wrap each message on server side inside tags like this :

<script>...</script>

(or anything else: <!----EOD-----> for example), and then, on client side, handle this situation: separate message based on

</script>

But I think, it shoud be in atmosphere core.

Owner

jfarcand commented Apr 16, 2012

Salut. Can you try the following:

Server Side

Add

        <init-param>
            <param-name>org.atmosphere.cpr.broadcastFilterClasses</param-name>
            <param-value>org.atmosphere.client.TrackMessageSizeFilter</param-value>
        </init-param>

Client side

When you create the request, add the trackMessageLength attributes

                var request = { url : document.location.toString(),
                    trackMessageLength : true,
                };

It may fix the issue. If not just let me know because it should :-) Thanks!

kirach commented Apr 16, 2012

I just tried your solution. Unfortunately, it doesn't work properly. It works from time to time in Chrome, and doesn't work in Firefox. Problem is on client side, because server sends messages with it's size and separated by delimiter. But client can't separate it properly. I'm trying to understand, what can be root of this bug, but without any success yet. If I'll found anything, I will notify you.

kirach commented Apr 16, 2012

It seems, that solution you have provided work only for situation, when server sends one message by several parts. But it doesn't work, when several messages are glued in one. I'll try to explain, why I think so.

ajaxRequest.onreadystatechange = function () {
    //...
    if (rq.transport == 'streaming') {
        //...
        var message = responseText.substring(rq.lastIndex, responseText.length);
        skipCallbackInvocation = _trackMessageSize(message, rq, _response);
        //...
        if (skipCallbackInvocation) {
            return;
        }
}

Above I provided important piece of code. skipCallbackInvocation becomes true, when size of message, received from server, is not equal to expected size(it can be in both situation: when several messages glued in one, or when one message is separated to several pieces by server). And if it is true, client doesn't process received message. It waits to next part of messages. That's why this solution doesn't work.

Owner

jfarcand commented Apr 16, 2012

Ok so we need to fix that (the same issue will happens with Websocket as well). Can you share your test case? Sen dit to jfarcand@apache.org

Owner

jfarcand commented Apr 17, 2012

OK a closer look at it and it will require a fair amount of work to make it work as the delimiter will need to be added before and after, e.g.

     | <message-size> | <message>  | <message-size> |

I will work on it for 1.0.0 release. For now you can add a BroadcastFilter that will wrap the message and on the client side, and inside your atmosphere.js callback parse the message yourself (I know that sucks, but that will make your application working). If you have the cycle and wants to collaborate, send me a patch :-) :-)

jfarcand added a commit that referenced this issue May 22, 2012

Owner

jfarcand commented May 22, 2012

Fixed and documented here

 https://github.com/Atmosphere/atmosphere/wiki/Tricks-and-Tips

(last item).

@jfarcand jfarcand closed this May 22, 2012

jfarcand added a commit that referenced this issue May 24, 2012

More fix for #286 Handling of "glued" messages. Make the token config…
…urable on server and client, use messageDelimiter as well for finding the end of the message

jfarcand added a commit that referenced this issue May 30, 2012

jfarcand added a commit that referenced this issue May 30, 2012

More fix for #286 Handling of "glued" messages. Make the token config…
…urable on server and client, use messageDelimiter as well for finding the end of the message

kirach commented Jun 16, 2012

I've just tried solution, you have provided. And, unfortunately, seems that it doesn't work. MessageLengthInterceptor now separates each message with <||>, it's hardcoded in this:

@Override
public AsyncIOWriter write(String data) throws IOException {
  response.write(data + "<||>");
    return this;
}

And TrackMessageSizeFilter tracks the lenght of each message, using |:

msg = msg.length() + "|" + msg;

But on client side (jquery.atmopshere.js) is used only one parameter: messageDelimiter. It's used both: when several messages come in one:

var messages = typeof(_response.responseBody) == 'string' ? _response.responseBody.split(_request.messageDelimiter) : new Array(_response.responseBody);

, and when one messages come by several parts:

if (request.trackMessageLength) {
  // The message length is the included within the message
  var messageStart = message.indexOf(request.messageDelimiter);

So, it's not impossible to prevent both of this situation.

@jfarcand jfarcand reopened this Jun 16, 2012

I agree with this analysis. I opened a similar bug: #421.
In fact, I'm facing both of these problems.

jfarcand added a commit that referenced this issue Jun 19, 2012

Owner

jfarcand commented Jun 19, 2012

OK I've broke the functionality when I refactored the class. Try the latest 1.0.0-SNAPSHOT and let me know how it goes.

@jfarcand jfarcand closed this Jun 19, 2012

jfarcand added a commit that referenced this issue Jun 19, 2012

jfarcand added a commit that referenced this issue Jun 21, 2012

kirach commented Jul 7, 2012

Sorry for the long delay. Just now I tried 1.0.0.beta2a, and have to say, that it doesn't work yet:( For now, after quick analyzing of code, it seems that MessageLengthInterceptor always adds bytes to the end of messages, even when message is String:

private byte[] end = END;

//...stuff

@Override
public AsyncIOWriter write(String data) throws IOException {
    response.write(data + end);
    return this;
}

That's why on client, every message in a result has [B@33637477 on the end instead of normal string representation of delimiter('|').

And now I totally do not understand conception of using delimiters. On the server side I have two available options:
org.atmosphere.client.MessageLengthInterceptor and org.atmosphere.client.TrackMessageSizeFilter. One for "glued" several messages, and the second for one message by several parts. But it uses same delimiters... And on client side it becomes more complicated.

What about merging this two functionalities in one? For example: on the server side interceptor adds delimiter to the end of each message. And the client side uses this delimiter to determine, if it should split message to several (if messages contains delimiter) or maybe it should wait for remainder of message(if message doesn't have delimiter at all). For now, we use this approach locally in our project, And I would be happy to provide pull request. But I don't know, if it affects some functionality of parts, we don't use in project(for example, writing data by byte[]).

Owner

jfarcand commented Jul 7, 2012

@kirach Wow, I deserve the goal medal of stupidity here. I've just fixed the bytes stuff.

Yes do me a pull request so I can look at what you did and investigate how I can definitively fix this issue.

Thanks for your feedback!

@jfarcand jfarcand reopened this Jul 7, 2012

Owner

jfarcand commented Jul 7, 2012

byte fix: ee3b416

had the same issue - please refer here for my fix:
#552

@jfarcand jfarcand closed this Aug 29, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment