replace preconditions with errors

[artemredkin]

Will address #49 as well

Before this PR, channel.close was called after Task promise was fulfilled. This introduced a slight delay before receiving .end and closing the channel. This in turn could lead to a situation where we could receive .head right after .end, which is allowed by HTTP protocol (keep-alive) but is not currently supported (pending connection pool work) by ResponseAccumulator. In order to address this, we need to close channel immediately after receiving .end.

[ianpartridge]

Looks good. It would be nice to have some tests which would have hit the preconditions and now catch a graceful failure.

[Lukasa]

This all looks great, thanks!

[artemredkin]

I'm trying to add a test, but it's not that straitforward :) NIO is pretty good at detecting when you do stupid things in your HTTPServerHandler :D

[artemredkin]

Test added

[weissi]

I don't think I agree with this patch. SwiftNIO will never (and if it did that'd be a serious bug) send you anything but exactly 1 .head, 0... .body, 1 .end. This is hiding bugs and isn't actionable to the user.

What was the motivation for this?

[artemredkin]

@weissi see #49, we receive head, end, head in some cases, but it's correct in terms of http protocol I think, but HTTPResponseAccumulator cannot handle second response on the same connection, I will investigate why channel was not closed

[artemredkin]

@weissi I think channel.close happens too late, we receive .end and succeed the promise but callback (see line 358 in HTTPHandler.swift) on it's future is not called before we receive another channelRead, this is how we get into this inconsistent state.

[weissi]

@artemredkin cool, but that should be fixed then, no?

[artemredkin]

Question is, why task futureResult callback is called so late? It seems we need to call context.close right away in all cases

[artemredkin]

@weissi I reverted back to preconditions (preconditions in redirect handler will be replaced by #67), real issue was that we were closing channel too late, I propose to close channel immediately instead of relying on futureResult callback.

[weissi]

@artemredkin mind fixing the PR description and adding a good explanation of when exactly this will help? Also we definitely need a test case that proves this is effective.

[artemredkin]

@weissi done

[artemredkin]

This is not needed at all, underlying problem is solved by NIO:
https://github.com/apple/swift-nio/pull/1093/files

[weissi]

@artemredkin released as swift-nio 2.6.1