HttpServerUpgradeHandler shouldn't wait for flush to reshape pipeline by bryce-anderson · Pull Request #7807 · netty/netty

bryce-anderson · 2018-03-23T15:34:41Z

Motivation:

There is a race between both flushing the upgrade response and receiving
more data before the flush ChannelPromise can fire the listener and reshape
the pipeline. Since We have already committed to an upgrade by writing the
upgrade response, we should immediately prepare for handling the next
protocol.

Modifications:

The pipeline reshaping logic in HttpServerUpgradeHandler has been moved
out of the ChannelFutureListener attached to the write of the upgrade
response and happens immediately after the writeAndFlush call, but
before the method returns.

Result:

The pipeline is no longer subject to receiving more data before the
pipeline has been reformed.

normanmaurer · 2018-03-23T15:37:36Z

Can we have a unit test ?

…

Am 23.03.2018 um 16:34 schrieb Bryce Anderson ***@***.***>: Motivation: There is a race between both flushing the upgrade response and receiving more data before the flush ChannelPromise can fire the listener and reshape the pipeline. Since We have already committed to an upgrade by writing the upgrade response, we should immediately prepare for handling the next protocol. Modifications: The pipeline reshaping logic in HttpServerUpgradeHandler has been moved out of the ChannelFutureListener attached to the write of the upgrade response and happens immediately after the writeAndFlush call, but before the method returns. Result: The pipeline is no longer subject to receiving more data before the pipeline has been reformed. Motivation: Explain here the context, and why you're making that change. What is the problem you're trying to solve. Modification: Describe the modifications you've done. Result: Fixes #. If there is no issue then describe the changes introduced by this PR. You can view, comment on, or merge this pull request online at: #7807 Commit Summary HttpServerUpgradeHandler shouldn't wait for flush to reshape pipeline File Changes M codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java (49) Patch Links: https://github.com/netty/netty/pull/7807.patch https://github.com/netty/netty/pull/7807.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

bryce-anderson · 2018-03-23T15:37:46Z

I think this should fix #7740, but am not 100% sure. Since the HttpServerUpgradeHandler is an HttpObjectAggregator, it should clear the inbound pipeline of the entire initial request, so unless the client sent and upgrade request and then tried to pipeline another (which is incorrect) the inbound socket channel should be clean at the point the pipeline is reshaped.

I'm also open to suggestions for tests. Currently there aren't any dedicated tests for HttpServerUpgradeHandler, but I may try and get one in.

normanmaurer · 2018-03-23T16:29:18Z

please add a license header.

normanmaurer · 2018-03-23T16:30:02Z

we use 4 spaces, please fix :)

normanmaurer · 2018-03-23T16:30:18Z

bryce-anderson · 2018-03-23T17:13:31Z

Thanks for the feedback @normanmaurer, I'll address it this afternoon. Cc'ing @ejona86 as he may have comments about correctness.

ejona86 · 2018-03-23T17:25:59Z

Didn't look in great detail, but this makes me feel better. There's really no reason I can think of to delay until the write is complete and I agree it just seems to add a race.

bryce-anderson · 2018-03-23T21:34:02Z

Thanks for the double check @ejona86. @normanmaurer, I think I addressed your comments. Thanks again for the review.

normanmaurer · 2018-03-25T15:43:21Z

Can we just use Collections.<CharSequence>emptyList() and so remove the dependency on the guava here ?

normanmaurer · 2018-03-25T15:44:27Z

Call super.write(....) to ensure we not "leak the message" and also to ensure the promise is notified.

This was deliberate so that we could be confident that the pipeline reshaping happened in this call stack irrespective of the promise being satisfied.

That said, perhaps it's not something that is likely to regress. And we do need to do something with the outbound message, good catch.

normanmaurer · 2018-03-25T15:45:11Z

just use Unpooled.copiedBuffer(upgradeString, CharsetUitl.US_ASCII)

normanmaurer · 2018-03-25T15:46:32Z

@bryce-anderson #3698 does not seem to be the correct issue to reference here... Can you check ?

bryce-anderson · 2018-03-26T15:01:23Z

@normanmaurer, sorry, you're right: the issue it attempts to fix is #7740.

nepthar · 2018-03-26T21:11:18Z

Is there a reason to delay this until the write succeeds? (I can't think of any)

The write hasn't necessarily succeeded yet as writeComplete is a ChannelPromise. We should enqueue the write before reshaping the pipeline as the incoming codec may try to write something when it gets added to the pipeline.

Maybe just delay the write by using ctx.eventLoop().execute(...) and schedule the write via it ?

That did the trick, thanks @normanmaurer. I'd tried to defer the satisfaction of the promise via indirection through the executor, but the flush() call would run it. Your suggestion is much more reasonable anyway.

Ah, shoot, my comment was really unclear - I wanted to note that we fireUserEventTriggered before the write is complete and ask if there could be a reason to delay that event until the write finished instead. I'm not sure what the contract around that user event is, but maybe some folks rely on that happening after the write?

nepthar · 2018-03-26T21:16:50Z

this test LGTM, but as a sanity check, does it fail without your change?

No, not anymore. Originally it did, but now that the write override on line 96 calls super.write as @normanmaurer requested, it will succeed either way. See his second round of comments.

I'm happy to take suggestions on how to test this cleanly, but the only thing I can think of is to drop the promise (what I had originally, and it did fail without my patch) or to store it somewhere else and fail it later with the former being bad form and the latter being a pretty ugly mess.

Using normans suggestion it now fails without the changes to HttpServerUpgradeHandler.

@bryce-anderson did you forget to push this change ?

@normanmaurer yes, I had. Sorry about that.

Scottmitch · 2018-03-28T02:44:54Z

write and flush shouldn't throw ... but consider including it in the try block below just to have less assumptions about releasing the event relative to behavior of this method.

Scottmitch · 2018-03-28T02:49:18Z

nit: just put this on the previous line. line length of 120 is permitted.

The bigger issue was that the comment was incomplete. Sorry about that.

Scottmitch · 2018-03-28T02:52:14Z

nit: remove throws Exception

normanmaurer · 2018-03-28T09:09:17Z

@bryce-anderson please address @Scottmitch s comments and ping me once done. I will pull in then.

Motivation: There is a race between both flushing the upgrade response and receiving more data before the flush ChannelPromise can fire and reshape the pipeline. Since We have already committed to an upgrade by writing the upgrade response, we need to be immediately prepared for handling the next protocol. Modifications: The pipeline reshaping logic in HttpServerUpgradeHandler has been moved out of the ChannelFutureListener attached to the write of the upgrade response and happens immediately after the writeAndFlush call, but before the method returns. Result: The pipeline is no longer subject to receiving more data before the pipeline has been reformed.

bryce-anderson · 2018-03-28T14:31:54Z

Thanks for the feedback @Scottmitch. @normanmaurer, should be ready. Thanks again for the reviews folks.

normanmaurer · 2018-03-28T14:40:34Z

Will merge once CI completes @bryce-anderson ... thanks!

bryce-anderson force-pushed the issue_3689 branch from eb31acc to 514be54 Compare March 23, 2018 16:29

normanmaurer requested changes Mar 23, 2018

View reviewed changes

bryce-anderson force-pushed the issue_3689 branch from 885934c to c4b1b8a Compare March 23, 2018 20:40

normanmaurer requested changes Mar 25, 2018

View reviewed changes

normanmaurer requested review from Scottmitch, carl-mastrangelo, ejona86, normanmaurer and trustin March 25, 2018 15:45

normanmaurer added the defect label Mar 25, 2018

bryce-anderson force-pushed the issue_3689 branch from c4b1b8a to d6f0c82 Compare March 26, 2018 14:47

ejona86 approved these changes Mar 26, 2018

View reviewed changes

nepthar approved these changes Mar 26, 2018

View reviewed changes

bryce-anderson force-pushed the issue_3689 branch 2 times, most recently from 0149216 to e374c04 Compare March 27, 2018 17:36

normanmaurer approved these changes Mar 27, 2018

View reviewed changes

normanmaurer added this to the 4.1.23.Final milestone Mar 27, 2018

Scottmitch approved these changes Mar 28, 2018

View reviewed changes

trustin approved these changes Mar 28, 2018

View reviewed changes

bryce-anderson force-pushed the issue_3689 branch from e374c04 to 799cc6e Compare March 28, 2018 14:22

bryce-anderson force-pushed the issue_3689 branch from 799cc6e to 0a6a2ab Compare March 28, 2018 14:30

normanmaurer merged commit b309271 into netty:4.1 Mar 28, 2018

normanmaurer mentioned this pull request Mar 28, 2018

HttpServerUpgrader should add the new handler before removing the old #7740

Closed

bryce-anderson deleted the issue_3689 branch March 28, 2018 18:20

Uh oh!

Conversation

bryce-anderson commented Mar 23, 2018 • edited by normanmaurer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

normanmaurer commented Mar 23, 2018 via email

Uh oh!

bryce-anderson commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryce-anderson commented Mar 23, 2018

Uh oh!

ejona86 commented Mar 23, 2018

Uh oh!

bryce-anderson commented Mar 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Mar 25, 2018

Uh oh!

bryce-anderson commented Mar 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Mar 28, 2018

Uh oh!

bryce-anderson commented Mar 28, 2018

Uh oh!

normanmaurer commented Mar 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bryce-anderson commented Mar 23, 2018 •

edited by normanmaurer

Loading

bryce-anderson commented Mar 23, 2018 •

edited

Loading