Ensure HTTPDecoder will not run into re-entrance issues and correctly… #427

normanmaurer · 2018-05-22T15:48:26Z

… forward bytes left after removal.

Motivation:

We need to ensure we correctly guard against re-entrancy for all cases. Also we did not correctly ensure we forward pending data after removal which could lead to missing data after upgrades.

Modifications:

pause the parser when we need to callout to the pipeline and so ensure we never fire events throught the pipeline while in callbacks of http_parser
correctly forward any pending bytes if an upgrade happened when the decoder is removed
Ensure HTTPUpgradeHandler only remove decoder after the full request is received
Add testcase provided by @weissi

Result:

Correctly handle upgrades and pending data. Fixes #422.

weissi · 2018-05-22T15:56:25Z

Sources/NIOHTTP1/HTTPDecoder.swift

    }

+    public func handlerRemoved(ctx: ChannelHandlerContext) {
+        if let buffer = self.cumulationBuffer?.slice(), buffer.readableBytes > 0, self.parser.upgrade == 1, ctx.channel.isActive {


why not && ? Don't hugely mind but usually we use && I think

Yeah, we prefer && over , except when we need to separate conditional bindings.

Also this is a really complex conditional, can we write a function to give this conditional a name?

weissi · 2018-05-22T15:57:58Z

Sources/NIOHTTP1/HTTPDecoder.swift

+    public func handlerRemoved(ctx: ChannelHandlerContext) {
+        if let buffer = self.cumulationBuffer?.slice(), buffer.readableBytes > 0, self.parser.upgrade == 1, ctx.channel.isActive {
+            self.cumulationBuffer = nil
+            ctx.fireChannelRead(NIOAny(buffer))


is this really safe? It really feels like there's a scenario where the next channel hander expects HTTP messages and this gets invoked during a channel close and then the next handler crashes

Any better idea ? I think we currently don’t know better

Yeah, don't do this if ctx.channel.isActive is false. That way if handlerRemoved is being invoked while the channel is being torn down you won't forward the data on.

I am confused... isn’t that exactly what I am doing here @Lukasa ?

I legit didn't even see that. That's part of why I wanted this conditional factored out. ;)

weissi · 2018-05-22T16:00:14Z

Sources/NIOHTTP1/HTTPDecoder.swift

-            self.cumulationBuffer = nil
+
+            if self.cumulationBuffer!.readableBytes == 0 {
+                // Its safe to just drop the cumulationBuffer as we not have any extra views into it that are represented as readerIndex / length.


... as we don't have any extra views

weissi

this looks really good actually! Any performance difference?

weissi · 2018-05-22T16:03:10Z

Sources/NIOHTTP1/HTTPDecoder.swift

+
+            // Ensure we pause the parser after this callback is complete so we can safely callout
+            // to the pipeline.
+            c_nio_http_parser_pause(parser, 1)


don't we need similar stuff for on_body or so?

ignore me, we've got that

normanmaurer · 2018-05-22T16:09:31Z

@weissi will run perf Tests after dinner

Lukasa · 2018-05-22T16:27:10Z

Sources/NIOHTTP1/HTTPDecoder.swift

    }

+    public func handlerRemoved(ctx: ChannelHandlerContext) {
+        if let buffer = self.cumulationBuffer?.slice(), buffer.readableBytes > 0, self.parser.upgrade == 1, ctx.channel.isActive {


Yeah, we prefer && over , except when we need to separate conditional bindings.

Lukasa · 2018-05-22T16:27:41Z

Sources/NIOHTTP1/HTTPDecoder.swift

    }

+    public func handlerRemoved(ctx: ChannelHandlerContext) {
+        if let buffer = self.cumulationBuffer?.slice(), buffer.readableBytes > 0, self.parser.upgrade == 1, ctx.channel.isActive {


Also this is a really complex conditional, can we write a function to give this conditional a name?

Lukasa · 2018-05-22T16:28:08Z

Sources/NIOHTTP1/HTTPDecoder.swift

+    public func handlerRemoved(ctx: ChannelHandlerContext) {
+        if let buffer = self.cumulationBuffer?.slice(), buffer.readableBytes > 0, self.parser.upgrade == 1, ctx.channel.isActive {
+            self.cumulationBuffer = nil
+            ctx.fireChannelRead(NIOAny(buffer))


Yeah, don't do this if ctx.channel.isActive is false. That way if handlerRemoved is being invoked while the channel is being torn down you won't forward the data on.

Lukasa · 2018-05-22T16:31:09Z

Sources/NIOHTTP1/HTTPUpgradeHandler.swift

 /// requests that we choose to punt on it entirely and not allow it. As it happens this is mostly fine:
 /// the odds of someone needing to upgrade midway through the lifetime of a connection are very low.
 public class HTTPServerUpgradeHandler: ChannelInboundHandler {
+


Why are we adding this blank line?

Will revert, left-over from some debugging Code that was there

Lukasa · 2018-05-22T16:32:43Z

Sources/NIOHTTP1/HTTPUpgradeHandler.swift

-                if bufferedMessages.count > 0 {
-                    ctx.fireChannelReadComplete()
+
+            return {


There's no need to heap allocate this, closing over the ChannelHandlerContext. Instead, factor this out to a separate function and just use a state variable.

I did it this way as otherwise we will need to store multiple things (like request, response headers, upgrader) and I thought this code looks just nicer + it’s not expected to run so frequently. WDYT?

we could try out luck with @convention(thin) ?

Eh, I guess that's fine? It feels super weird to me, but ok. We can't use @convention(thin) because as @normanmaurer notes this actually closes over a bunch of state.

We could encapsulate this in a struct instead, to avoid the heap allocation.

I am happy with the struct as well I just thought this one is a bit nicer and it should not matter. That said I am fine either way, just tell me what to do :)

For now we can allocate the closure, we can always remove it later.

Lukasa · 2018-05-22T16:33:35Z

Sources/NIOHTTP1/HTTPUpgradeHandler.swift

+                notUpgrading(ctx: ctx, data: data)
+                return
+            case .body(_):
+                // ignore


Can you mark this with a TODO that indicates that in a future NIO version we want to add an API where we deliver this to the upgrader in some form?

Lukasa · 2018-05-22T16:35:01Z

Sources/NIOHTTP1/HTTPUpgradeHandler.swift

+            }
+        } else {
+            // We should only ever see a request header: by the time the body comes in we should
+            // be out of the pipeline. Anything else is an error.


This comment needs rewording as it's no longer accurate. Maybe:

We should decide if we're going to upgrade based on the first request header: if we aren't upgrading, by the time the body comes in we should be out of the pipeline. That means that if we don't think we're upgrading, the only thing we should see is a request head. Anything else in an error.

normanmaurer · 2018-05-23T07:15:07Z

Alright did run our perf-suite...

master:

measuring: no-net_http1_10k_reqs_1_conn: 0.193889021873474, 0.19300103187561, 0.189878940582275, 0.202571034431458, 0.195896029472351, 0.197288036346436, 0.202276945114136, 0.193497061729431, 0.199617028236389, 0.190865039825439,
measuring: http1_10k_reqs_1_conn: 0.559623003005981, 0.557006001472473, 0.561303019523621, 0.561969041824341, 0.565063953399658, 0.564036011695862, 0.570590972900391, 0.555812001228333, 0.560003042221069, 0.556936979293823,
measuring: http1_10k_reqs_100_conns: 0.602265000343323, 0.603261947631836, 0.60293710231781, 0.606209993362427, 0.620349049568176, 0.612009048461914, 0.66516900062561, 0.610424995422363, 0.606754899024963, 0.618997097015381,

With this pr:

measuring: no-net_http1_10k_reqs_1_conn: 0.231523990631104, 0.221113085746765, 0.226511001586914, 0.227283000946045, 0.221638917922974, 0.222470045089722, 0.224020957946777, 0.230138063430786, 0.223469018936157, 0.220298051834106,
measuring: http1_10k_reqs_1_conn: 0.599761009216309, 0.598889946937561, 0.595099091529846, 0.598055958747864, 0.602496981620789, 0.640310049057007, 0.588855981826782, 0.594573974609375, 0.610931038856506, 0.599555015563965,
measuring: http1_10k_reqs_100_conns: 0.64155101776123, 0.639289975166321, 0.645186901092529, 0.649052023887634, 0.64255690574646, 0.644281029701233, 0.637521982192993, 0.642242074012756, 0.65774393081665, 0.642369031906128,

normanmaurer · 2018-05-23T07:16:39Z

So it seems like this definitely harm the perf a little bit, which I think is not really unexpected with the extra branching etc.

normanmaurer · 2018-05-23T07:17:42Z

@weissi @Lukasa ^^

weissi · 2018-05-23T07:38:46Z

@normanmaurer ouch, raw performance, no networking (no-net_http1_10k_reqs_1_conn) down 16% 😢.

normanmaurer · 2018-05-23T07:40:29Z

@weissi yeah... that said I am not sure there is much we can do here (except use a pending list again)... I am open to suggestions tho :)

normanmaurer · 2018-05-23T07:47:51Z

Sources/NIOHTTP1/HTTPDecoder.swift

-            self.state.currentError = HTTPParserError.httpError(fromCHTTPParserErrno: http_errno(rawValue: httpError))!
-            ctx.fireErrorCaught(self.state.currentError!)
+        // Also take into account that we may have called c_nio_http_parser_pause(...)
+        guard httpError != 0 && httpError != HPE_PAUSED.rawValue else {


@weissi I guess it not really matters much for perf stuff but we may want to consider switching these as most of the time it will be HPE_PAUSED in reality so the first test is a waste mostly.

reordered this one

weissi · 2018-05-23T07:58:02Z

@normanmaurer is it faster with the callout list?

normanmaurer · 2018-05-23T08:00:37Z

@weissi I will check in a few need to fix some other stuff first :(

normanmaurer · 2018-05-23T08:42:13Z

After 61b3149 :

measuring: no-net_http1_10k_reqs_1_conn: 0.2206209897995, 0.221046090126038, 0.211659908294678, 0.212454080581665, 0.210615038871765, 0.208593010902405, 0.209282994270325, 0.208821058273315, 0.209628939628601, 0.209208965301514,
measuring: http1_10k_reqs_1_conn: 0.580152988433838, 0.57462203502655, 0.577759981155396, 0.582140922546387, 0.578655004501343, 0.572811961174011, 0.572220087051392, 0.578151941299438, 0.583968043327332, 0.611997961997986,
measuring: http1_10k_reqs_100_conns: 0.62391996383667, 0.626338005065918, 0.628537058830261, 0.629424929618835, 0.61939001083374, 0.638810038566589, 0.632416963577271, 0.63428795337677, 0.626121044158936, 0.628298044204712,

So not too bad imho @weissi

weissi · 2018-05-23T08:45:27Z

@normanmaurer much better, 8%. I'm probably happy enough with this

normanmaurer · 2018-05-23T08:47:27Z

@weissi you want me to try a list as well still ?

weissi · 2018-05-23T08:48:36Z

@normanmaurer up to you but I think it could be a follow-up. Because this fixes a correctness issue so I think we should get it in asap

Lukasa · 2018-05-23T08:47:56Z

Sources/NIOHTTP1/HTTPDecoder.swift

+        guard self.parser.upgrade == 1 && ctx.channel.isActive else {
+            return nil
+        }
+        if let buffer = self.cumulationBuffer?.slice(), buffer.readableBytes > 0 {


Out of interest, why are we slicing the cumulation buffer here?

@Lukasa I just thought it may be a bit nicer to consume. That said we not need to slice it. Happy to remove if you think we should

Unexplained slicing without a code comment is the kind of thing that we'll be real nervous to change in 6 months time. :D I think either remove it or put a comment explaining why it's there.

added a comment ;)

normanmaurer · 2018-05-23T08:49:22Z

@weissi sounds good to me... waiting for @Lukasa then :) Also please don't forget to approve ;)

Lukasa

WFM.

@weissi

… forward bytes left after removal. Motivation: We need to ensure we correctly guard against re-entrancy for all cases. Also we did not correctly ensure we forward pending data after removal which could lead to missing data after upgrades. Modifications: - pause the parser when we need to callout to the pipeline and so ensure we never fire events throught the pipeline while in callbacks of http_parser - correctly forward any pending bytes if an upgrade happened when the decoder is removed - Ensure HTTPUpgradeHandler only remove decoder after the full request is received - Add testcase provided by @weissi Result: Correctly handle upgrades and pending data. Fixes apple#422.

normanmaurer · 2018-05-23T09:21:35Z

I squashed, rebased and pushed again

weissi

thanks, looks great!

normanmaurer requested review from Lukasa and weissi May 22, 2018 15:48

normanmaurer force-pushed the http_decoder branch 2 times, most recently from 930731a to 4855599 Compare May 22, 2018 15:49

weissi reviewed May 22, 2018

View reviewed changes

weissi requested changes May 22, 2018

View reviewed changes

Lukasa requested changes May 22, 2018

View reviewed changes

weissi added this to the 1.7.2 milestone May 22, 2018

normanmaurer force-pushed the http_decoder branch 2 times, most recently from ae83e9c to 3051625 Compare May 22, 2018 18:55

normanmaurer commented May 23, 2018

View reviewed changes

Lukasa reviewed May 23, 2018

View reviewed changes

Lukasa approved these changes May 23, 2018

View reviewed changes

Lukasa added the 🔨 semver/patch No public API change. label May 23, 2018

normanmaurer force-pushed the http_decoder branch from 61e6841 to 5522789 Compare May 23, 2018 09:21

weissi approved these changes May 23, 2018

View reviewed changes

weissi merged commit 398b950 into apple:master May 23, 2018

normanmaurer deleted the http_decoder branch May 23, 2018 14:58

Ensure HTTPDecoder will not run into re-entrance issues and correctly… #427

Ensure HTTPDecoder will not run into re-entrance issues and correctly… #427

Uh oh!

Conversation

normanmaurer commented May 22, 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!

weissi left a comment

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 May 22, 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!

normanmaurer commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

normanmaurer commented May 23, 2018

Uh oh!

normanmaurer commented May 23, 2018

Uh oh!

weissi commented May 23, 2018

Uh oh!

normanmaurer commented May 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weissi commented May 23, 2018

Uh oh!

normanmaurer commented May 23, 2018

Uh oh!

normanmaurer commented May 23, 2018 •

edited

Loading

weissi commented May 23, 2018 •

edited

Loading