Add test for HTTP1 request with large header #658
Conversation
dnadoba
added
the
semver/patch
| @@ -156,6 +157,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler { | |||
| metadata: req.requestFramingMetadata | |||
| ) | |||
| self.run(action, context: context) | |||
| promise?.succeed(()) | |||
There was a problem hiding this comment.
This doesn't seem appropriate. We should presumably send the promise on with the appropriate data, rather than silently succeeding it here.
There was a problem hiding this comment.
Good catch. I have reverted it now. I though I needed it because try channel.writeOutbound(request) would block indefinitely but I have now replaced it with channel.writeAndFlush(request, promise: nil) which doesn't block and is closer to the production code. efa86b5
|
|
||
| init(connection: HTTP1Connection, eventLoop: EventLoop, logger: Logger) { | ||
| self.connection = connection | ||
| var onRequestCompleted: () -> Void = {} |
There was a problem hiding this comment.
Why are we adding this indirection here?
There was a problem hiding this comment.
We need it to break the dependency between HTTP1Connection and HTTP1ClientChannelHandler. HTTP1ClientChannelHandler only needs a reference to HTTP1Connection to get the connection id for logging and to signal that the last request is complete. The connection.id is now passed as connectionIdLoggerMetadata: Logger.MetadataValue in the init. connection.taskComplete() is replaced with onRequestCompleted() indirection.
I think in the long run we should replace this with a write promise we pass together with the request here:
This will cost us an additional allocation per request but this should be fine.
There was a problem hiding this comment.
Yes, this was my reaction: having unstructured callbacks like this is a bit of a worry for me. Promises are a substantial improvement.
There was a problem hiding this comment.
Is there any reason not to make that change soon?
There was a problem hiding this comment.
HTTP2 currently uses the same state machine and doesn't need to store separate promise because it creates a new stream and therefore a new channel for each request. It uses the channel.closeFuture to detect that the request is done (well it will soon #657). So it doesn't need a separate promise but it also doesn't hurt either.
At the end nothing technically is blocking this, it's just work that need to be done.
There was a problem hiding this comment.
Ok, I'm asking only because I suspect this mystery-meat callback will live here forever if we don't make the change now.
There was a problem hiding this comment.
So I have tried to do that. The problem I run into is that it is currently not really used like a promise and may never be called. It is used to signal that the connection is again able to receive further request which it may never be for various reasons. This case is currently handled by closing the channel instead of calling this callback. At the end it just calls down to connection.delegate.http1ConnectionReleased(connection)
I think changing and testing this is quite some effort which I would like not to do right now.
There was a problem hiding this comment.
I have at least renamed it to onConnectionIdle to better explain what it actually does: 5f5dc1e
dnadoba
force-pushed
the
dn-large-header-tests-http1
branch
from
0cc2adb
to
efa86b5
Compare
Motivation
We currently don't handle large headers well which trigger a channel writability change event.
Modification
HTTP1ConnectionHandlerto make it more testableResult
We can reliably reproduce the large request header issue in an integration and unit test.
Note that the actual fix is not included to make reviewing easier and will come in a follow up PR.