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

Already on GitHub? Sign in to your account

Added detection of split token #481

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

eric-vautour commented Nov 12, 2013

Looks like when we call ssl_client.readpartial(1024), it may split the token used by the BufferedTokenizer. Thus we get entries like:

@tokenizer.extract(something-before\r)
@tokenizer.extract(\nsomethin-after)

And thus BufferedTokenizer will not parse on the \r\n, and will result in invalid JSON
{somestuff}\r
\n{somotherstuff}

Results in JSON.parse('{somestuff}{someotherstuff}')

Coverage Status

Coverage increased (+0.22%) when pulling 3770136 on eric-vautour:master into 6ad7fe3 on sferik:master.

@sferik sferik commented on the diff Nov 18, 2013

lib/twitter/streaming/response.rb
@@ -23,6 +23,11 @@ def on_headers_complete(headers)
end
def on_body(data)
+ # Ended with half a token \r and not \r\n and thus BufferedTokenizer will
+ # not split the line correctly. We need to ensure that each chunk of data passed
+ # to BufferedTokenizer does NOT end with \r
+ data += "\n" if data[-1] == "\r"
@sferik

sferik Nov 18, 2013

Owner

Won’t this cause there to be an extra \n?

@eric-vautour

eric-vautour Nov 18, 2013

Contributor

Yes.. there would be an extra \n, there has got to be a better solution but unfortunately, I don't have the time to look into it right now.

Owner

sferik commented Nov 18, 2013

Can you please add specs to this pull request?

Owner

sferik commented Nov 28, 2013

I cherry-picked 0c190bc into master and resolved the split token issue by updating buftok in 64dc08f.

@sferik sferik closed this Nov 28, 2013

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