fix errors received when holding messages in HTTPServerPipelineHandler #314
Conversation
weissi
force-pushed
the
jw-error-when-pipelining
branch
from
cc9fd17 to
e8c1629
Compare
| ctx.fireErrorCaught(error) | ||
| return | ||
| } | ||
| self.eventBuffer.append(.error(httpError)) |
There was a problem hiding this comment.
This isn’t right: if we’re not expecting a response this will never unbuffer.
There was a problem hiding this comment.
Yes, agree. I wasn’t quite so sure how exactly we want to handle this situation so I didn’t really want to write all the code&tests. We should discuss and then I’ll implement it in this pr if that works for you.
So do you think we should generally buffer the http errors and deliver them as soon as we can in a way that’s semantically similar to having received one request after the other? I think that’s probably best.
Said that the server protocol error handler will probably still need to become and outbound handler to see if the response has started already, right?
|
Generally fine with this approach, just left a correctness note. |
weissi
force-pushed
the
jw-error-when-pipelining
branch
from
e8c1629 to
8ebcaed
Compare
| _ = ctx.eventLoop.scheduleTask(in: .seconds(10) /* that'll never actually execute */) { | ||
| XCTAssertEqual(.none, self.nextExpected) | ||
| self.nextExpected = .head | ||
| ctx.writeAndFlush(self.wrapOutboundOut(HTTPServerResponsePart.end(nil)), promise: nil) |
There was a problem hiding this comment.
If this will never execute, shouldn't this XCTFail instead of having production code in it?
| try channel.writeInbound(buffer) | ||
| try channel.close().wait() | ||
| } catch HTTPParserError.invalidContentLength { | ||
| // This error is expected |
There was a problem hiding this comment.
Are you sure you're hitting this error? Because I don't think you are. 😁
There was a problem hiding this comment.
🤔 deffo not, probably too much copy and paste
Motivation: We had a bug which is happens in the combination of these states: - we held a request in the pipelining handler (because we're procesing a previous one) - a http handler error happened whilst a response's `.head` had already been sent (but not yet the `.end`) - the HTTPServerProtocolErrors handler is in use That would lead to this situation: - the error isn't held by the pipelining handler - the error handler then just sends a full response (`.head` and `.end`) but the actual http server already send a `.head`. So all in all, we sent `.head`, `.head`, `.end` which is illegal - the pipelining handler didn't notice this and beause it saw an `.end` it would send through the next requst - now the http server handler is in the situation that it gets `.head`, `.head` too (which is illegal) Modifications: - hold HTTP errors in the pipelining handler too Result: - more correctness
weissi
force-pushed
the
jw-error-when-pipelining
branch
from
8ebcaed to
274a203
Compare
Motivation:
We had a bug which is happens in the combination of these states:
previous one)
.headhad alreadybeen sent (but not yet the
.end)That would lead to this situation:
.headand.end)but the actual http server already send a
.head. So all in all, wesent
.head,.head,.endwhich is illegal.endit would send through the next requst
.head,.headtoo (which is illegal)Modifications:
Result: