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

Do not flush OutputStream to avoid contamination #998

Merged
merged 3 commits into from
May 22, 2015

Conversation

itugs
Copy link
Contributor

@itugs itugs commented May 22, 2015

Related with #997

BinaryJedisPubSub#process use Client#getObjectMultiBulkReply and it flush output stream before read from socket. This cause contamination of outputbuffer and redis close client connection due to Protocol Error. Using Client#getRawObjectMultiBulkReply will fix this.

JedisPubSub already use it. So if we change that test case with JedisPubSub it do not fail.
Maybe there was a missing.

Here is TCP dump log of test case provided #996

11:16:26.417775 IP localhost.61043 > localhost.6379: Flags [P.], seq 1:34, ack 1, win 12759, options [nop,nop,TS val 596420889 ecr 596420884], length 33
E..Un.@.@............s....o.... ..1..I.....
#...#...*2  <--- first subscribe
$9
SUBSCRIBE
$8
whatever

11:16:26.417800 IP localhost.6379 > localhost.61043: Flags [.], ack 34, win 12758, options [nop,nop,TS val 596420889 ecr 596420889], length 0
E..4(A@.@..............s... ..o...1..(.....
#...#...
11:16:26.417844 IP localhost.6379 > localhost.61043: Flags [P.], seq 1:38, ack 34, win 12758, options [nop,nop,TS val 596420889 ecr 596420889], length 37
E..Y.Z@.@..............s... ..o...1..M.....
#...#...*3
$9
subscribe
$8
whatever
:1

// omitted

11:16:26.436680 IP localhost.61043 > localhost.6379: Flags [P.], seq 220:222, ack 236, win 12752, options [nop,nop,TS val 596420907 ecr 596420907], length 2
E..6q4@.@............s....p.......1..*.....
#..+#..+*2     <----  partial tx due to contamination

11:16:26.436692 IP localhost.61043 > localhost.6379: Flags [P.], seq 222:264, ack 236, win 12752, options [nop,nop,TS val 596420907 ecr 596420907], length 42
E..^r.@.@............s....p.......1..R.....
#..+#..+*2     <----  normal tx
051a2777-c81f-455c-a607-4f48fe5a59ce

// omitted

11:16:26.436715 IP localhost.6379 > localhost.61043: Flags [P.], seq 236:283, ack 306, win 12749, options [nop,nop,TS val 596420907 ecr 596420907], length 47
E..c.|@.@..............s......p...1..W.....
#..+#..+-ERR Protocol error: invalid multibulk length   <--- Protocol error

// omitted

11:16:26.436786 IP localhost.6379 > localhost.61043: Flags [R], seq 2952692539, win 0, length 0
E..(+.@.@..............s...;....P.......     <--- RST

@marcosnils
Copy link
Contributor

LGTM. Whenever another collaborator approves we should definitely merge.

@itugs thanks for the thorough investigation.

@HeartSaVioR
Copy link
Contributor

I've just try tcpdump and found there's some issue but I don't think about difference of BinaryJedisPubSub and JedisPubSub.
Amazing works!

@xetorthio
Copy link
Contributor

Yes. Good catch! Lets merge it!

@marcosnils
Copy link
Contributor

@itugs did you run the tests before commiting?. Seems like something else broke because if this change

@HeartSaVioR
Copy link
Contributor

@itugs BinaryJedisPubSub.proceed() and BinaryJedisPubSub.proceedWithPatterns() should execute flush() after calling psubscribe() / subscribe().

@itugs
Copy link
Contributor Author

itugs commented May 22, 2015

Oh, I'll follow that test failure. Sorry to bother you.

@itugs
Copy link
Contributor Author

itugs commented May 22, 2015

I revised and PublishSubscribeCommandsTest successed.
Some tests are still fail but I think it's due to env of CI.

@HeartSaVioR
Copy link
Contributor

No problem. We also know random test failures, and we re-run tests.

@HeartSaVioR
Copy link
Contributor

OK, test failure issue is gone.

@marcosnils
Copy link
Contributor

Amazing! @HeartSaVioR. Can you merge?

@HeartSaVioR
Copy link
Contributor

@marcosnils I'll merge it within 1 hour. :)

HeartSaVioR added a commit that referenced this pull request May 22, 2015
Do not flush OutputStream to avoid contamination
@HeartSaVioR HeartSaVioR merged commit 68b6372 into redis:master May 22, 2015
@HeartSaVioR
Copy link
Contributor

Merged to master, 2.7, 2.8 respectively.

Thanks for your amazing work! @itugs

@itugs
Copy link
Contributor Author

itugs commented May 22, 2015

Thanks! I'm very exciting to be contributing to Jedis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants