Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for HTTP1 request with large header #658

Merged
merged 8 commits into from
Jan 26, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
didSet {
if let newRequest = self.request {
var requestLogger = newRequest.logger
requestLogger[metadataKey: "ahc-connection-id"] = "\(self.connection.id)"
requestLogger[metadataKey: "ahc-el"] = "\(self.connection.channel.eventLoop)"
requestLogger[metadataKey: "ahc-connection-id"] = self.connectionIdLoggerMetadata
requestLogger[metadataKey: "ahc-el"] = "\(self.eventLoop)"
self.logger = requestLogger

if let idleReadTimeout = newRequest.requestOptions.idleReadTimeout {
Expand All @@ -59,15 +59,15 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {

private let backgroundLogger: Logger
private var logger: Logger
private let eventLoop: EventLoop
private let connectionIdLoggerMetadata: Logger.MetadataValue

let connection: HTTP1Connection
let eventLoop: EventLoop

init(connection: HTTP1Connection, eventLoop: EventLoop, logger: Logger) {
self.connection = connection
var onRequestCompleted: () -> Void = {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we adding this indirection here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was my reaction: having unstructured callbacks like this is a bit of a worry for me. Promises are a substantial improvement.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason not to make that change soon?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm asking only because I suspect this mystery-meat callback will live here forever if we don't make the change now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have at least renamed it to onConnectionIdle to better explain what it actually does: 5f5dc1e

init(eventLoop: EventLoop, backgroundLogger: Logger, connectionIdLoggerMetadata: Logger.MetadataValue) {
self.eventLoop = eventLoop
self.backgroundLogger = logger
self.logger = self.backgroundLogger
self.backgroundLogger = backgroundLogger
self.logger = backgroundLogger
self.connectionIdLoggerMetadata = connectionIdLoggerMetadata
}

func handlerAdded(context: ChannelHandlerContext) {
Expand Down Expand Up @@ -108,6 +108,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {

let action = self.state.writabilityChanged(writable: context.channel.isWritable)
self.run(action, context: context)
context.fireChannelWritabilityChanged()
}

func channelRead(context: ChannelHandlerContext, data: NIOAny) {
Expand Down Expand Up @@ -156,6 +157,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
metadata: req.requestFramingMetadata
)
self.run(action, context: context)
promise?.succeed(())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem appropriate. We should presumably send the promise on with the appropriate data, rather than silently succeeding it here.

Copy link
Collaborator Author

@dnadoba dnadoba Jan 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

func read(context: ChannelHandlerContext) {
Expand Down Expand Up @@ -274,7 +276,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
if shouldClose {
context.close(promise: nil)
} else {
self.connection.taskCompleted()
self.onRequestCompleted()
}

oldRequest.succeedRequest(buffer)
Expand All @@ -286,7 +288,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {

context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: writePromise)
case .informConnectionIsIdle:
self.connection.taskCompleted()
self.onRequestCompleted()
oldRequest.succeedRequest(buffer)
}

Expand All @@ -303,7 +305,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
oldRequest.fail(error)

case .informConnectionIsIdle:
self.connection.taskCompleted()
self.onRequestCompleted()
oldRequest.fail(error)

case .failWritePromise(let writePromise):
Expand All @@ -328,6 +330,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
// we must check if the request is still present here.
guard let request = self.request else { return }
request.requestHeadSent()

request.resumeRequestBodyStream()
} else {
context.write(self.wrapOutboundOut(.head(head)), promise: nil)
Expand Down Expand Up @@ -434,6 +437,11 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
}
}

#if swift(>=5.6)
@available(*, unavailable)
extension HTTP1ClientChannelHandler: Sendable {}
#endif

extension HTTP1ClientChannelHandler: HTTPRequestExecutor {
func writeRequestBodyPart(_ data: IOData, request: HTTPExecutableRequest, promise: EventLoopPromise<Void>?) {
if self.eventLoop.inEventLoop {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,13 @@ final class HTTP1Connection {
}

let channelHandler = HTTP1ClientChannelHandler(
connection: self,
eventLoop: channel.eventLoop,
logger: logger
backgroundLogger: logger,
connectionIdLoggerMetadata: "\(self.id)"
)
channelHandler.onRequestCompleted = {
self.taskCompleted()
}

try sync.addHandler(channelHandler)
} catch {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ extension HTTP1ClientChannelHandlerTests {
("testFailHTTPRequestWithContentLengthBecauseOfChannelInactiveWaitingForDemand", testFailHTTPRequestWithContentLengthBecauseOfChannelInactiveWaitingForDemand),
("testWriteHTTPHeadFails", testWriteHTTPHeadFails),
("testHandlerClosesChannelIfLastActionIsSendEndAndItFails", testHandlerClosesChannelIfLastActionIsSendEndAndItFails),
("testChannelBecomesNonWritableDuringHeaderWrite", testChannelBecomesNonWritableDuringHeaderWrite),
]
}
}
31 changes: 31 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,37 @@ class HTTP1ClientChannelHandlerTests: XCTestCase {
XCTAssertTrue(error is FailEndHandler.Error)
}
}

func testChannelBecomesNonWritableDuringHeaderWrite() throws {
try XCTSkipIf(true, "this currently fails and will be fixed in follow up PR")
final class ChangeWritabilityOnFlush: ChannelOutboundHandler {
typealias OutboundIn = Any
func flush(context: ChannelHandlerContext) {
context.flush()
(context.channel as! EmbeddedChannel).isWritable = false
context.fireChannelWritabilityChanged()
}
}
let eventLoopGroup = EmbeddedEventLoopGroup(loops: 1)
let eventLoop = eventLoopGroup.next() as! EmbeddedEventLoop
let handler = HTTP1ClientChannelHandler(
eventLoop: eventLoop,
backgroundLogger: Logger(label: "no-op", factory: SwiftLogNoOpLogHandler.init),
connectionIdLoggerMetadata: "test connection"
)
let channel = EmbeddedChannel(handlers: [
ChangeWritabilityOnFlush(),
handler,
], loop: eventLoop)
try channel.connect(to: .init(ipAddress: "127.0.0.1", port: 80)).wait()

let request = MockHTTPExecutableRequest()
// non empty body is important to trigger this bug as we otherwise finish the request in a single flush
request.requestFramingMetadata.body = .fixedSize(1)
request.raiseErrorIfUnimplementedMethodIsCalled = false
try channel.writeOutbound(request)
XCTAssertEqual(request.events.map(\.kind), [.willExecuteRequest, .requestHeadSent])
}
}

class TestBackpressureWriter {
Expand Down
2 changes: 1 addition & 1 deletion Tests/AsyncHTTPClientTests/HTTPClientBase.swift
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class XCTestCaseHTTPClientTestsBaseClass: XCTestCase {
var backgroundLogStore: CollectEverythingLogHandler.LogStore!

var defaultHTTPBinURLPrefix: String {
return "http://localhost:\(self.defaultHTTPBin.port)/"
self.defaultHTTPBin.baseURL
}

override func setUp() {
Expand Down
13 changes: 12 additions & 1 deletion Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,18 @@ internal final class HTTPBin<RequestHandler: ChannelInboundHandler> where
return "https"
}
}()
return "\(scheme)://localhost:\(self.port)/"
let host: String = {
switch self.socketAddress {
case .v4:
return self.socketAddress.ipAddress!
case .v6:
return "[\(self.socketAddress.ipAddress!)]"
case .unixDomainSocket:
return self.socketAddress.pathname!
}
}()

return "\(scheme)://\(host):\(self.port)/"
}

private let mode: Mode
Expand Down
1 change: 1 addition & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ extension HTTPClientTests {
("testRequestWithHeaderTransferEncodingIdentityDoesNotFail", testRequestWithHeaderTransferEncodingIdentityDoesNotFail),
("testMassiveDownload", testMassiveDownload),
("testShutdownWithFutures", testShutdownWithFutures),
("testMassiveHeaderHTTP1", testMassiveHeaderHTTP1),
]
}
}
15 changes: 15 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3363,4 +3363,19 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass {
let httpClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup))
XCTAssertNoThrow(try httpClient.shutdown().wait())
}

func testMassiveHeaderHTTP1() throws {
try XCTSkipIf(true, "this currently crashes and will be fixed in follow up PR")
var request = try HTTPClient.Request(url: defaultHTTPBin.baseURL, method: .POST)
// add ~64 KB header
let headerValue = String(repeating: "0", count: 1024)
for headerID in 0..<64 {
request.headers.replaceOrAdd(name: "larg-header-\(headerID)", value: headerValue)
}

// non empty body is important to trigger this bug as we otherwise finish the request in a single flush
request.body = .byteBuffer(ByteBuffer(bytes: [0]))

XCTAssertNoThrow(try defaultClient.execute(request: request).wait())
}
}
38 changes: 19 additions & 19 deletions Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
// for the first eight requests, the pool should try to create new connections.

for _ in 0..<8 {
let mockRequest = MockHTTPRequest(eventLoop: elg.next())
let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next())
let request = HTTPConnectionPool.Request(mockRequest)
let action = state.executeRequest(request)
guard case .createConnection(let connectionID, let connectionEL) = action.connection else {
Expand All @@ -53,7 +53,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
// the next eight requests should only be queued.

for _ in 0..<8 {
let mockRequest = MockHTTPRequest(eventLoop: elg.next())
let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next())
let request = HTTPConnectionPool.Request(mockRequest)
let action = state.executeRequest(request)
guard case .none = action.connection else {
Expand Down Expand Up @@ -120,7 +120,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
// for the first eight requests, the pool should try to create new connections.

for _ in 0..<8 {
let mockRequest = MockHTTPRequest(eventLoop: elg.next())
let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next())
let request = HTTPConnectionPool.Request(mockRequest)
let action = state.executeRequest(request)
guard case .createConnection(let connectionID, let connectionEL) = action.connection else {
Expand All @@ -136,7 +136,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
// the next eight requests should only be queued.

for _ in 0..<8 {
let mockRequest = MockHTTPRequest(eventLoop: elg.next())
let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next())
let request = HTTPConnectionPool.Request(mockRequest)
let action = state.executeRequest(request)
guard case .none = action.connection else {
Expand Down Expand Up @@ -181,7 +181,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
retryConnectionEstablishment: true
)

let mockRequest = MockHTTPRequest(eventLoop: elg.next())
let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next())
let request = HTTPConnectionPool.Request(mockRequest)

let action = state.executeRequest(request)
Expand Down Expand Up @@ -239,7 +239,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
retryConnectionEstablishment: true
)

let mockRequest = MockHTTPRequest(eventLoop: elg.next())
let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next())
let request = HTTPConnectionPool.Request(mockRequest)

let executeAction = state.executeRequest(request)
Expand Down Expand Up @@ -276,7 +276,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
retryConnectionEstablishment: true
)

let mockRequest = MockHTTPRequest(eventLoop: elg.next())
let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next())
let request = HTTPConnectionPool.Request(mockRequest)

let executeAction = state.executeRequest(request)
Expand Down Expand Up @@ -310,7 +310,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
XCTAssertEqual(cleanupContext.connectBackoff, [])

// 4. execute another request
let finalMockRequest = MockHTTPRequest(eventLoop: elg.next())
let finalMockRequest = MockHTTPScheduableRequest(eventLoop: elg.next())
let finalRequest = HTTPConnectionPool.Request(finalMockRequest)
let failAction = state.executeRequest(finalRequest)
XCTAssertEqual(failAction.connection, .none)
Expand Down Expand Up @@ -339,7 +339,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
return XCTFail("Expected to still have connections available")
}

let mockRequest = MockHTTPRequest(eventLoop: eventLoop)
let mockRequest = MockHTTPScheduableRequest(eventLoop: eventLoop)
let request = HTTPConnectionPool.Request(mockRequest)
let action = state.executeRequest(request)

Expand All @@ -359,7 +359,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
var queuer = MockRequestQueuer()
for _ in 0..<100 {
let eventLoop = elg.next()
let mockRequest = MockHTTPRequest(eventLoop: eventLoop, requiresEventLoopForChannel: false)
let mockRequest = MockHTTPScheduableRequest(eventLoop: eventLoop, requiresEventLoopForChannel: false)
let request = HTTPConnectionPool.Request(mockRequest)
let action = state.executeRequest(request)

Expand Down Expand Up @@ -418,7 +418,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {

// 10% of the cases enforce the eventLoop
let elRequired = (0..<10).randomElement().flatMap { $0 == 0 ? true : false }!
let mockRequest = MockHTTPRequest(eventLoop: reqEventLoop, requiresEventLoopForChannel: elRequired)
let mockRequest = MockHTTPScheduableRequest(eventLoop: reqEventLoop, requiresEventLoopForChannel: elRequired)
let request = HTTPConnectionPool.Request(mockRequest)

let action = state.executeRequest(request)
Expand Down Expand Up @@ -482,7 +482,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
XCTAssertEqual(connections.parked, 8)

// close a leased connection == abort
let mockRequest = MockHTTPRequest(eventLoop: elg.next())
let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next())
let request = HTTPConnectionPool.Request(mockRequest)
guard let connectionToAbort = connections.newestParkedConnection else {
return XCTFail("Expected to have a parked connection")
Expand Down Expand Up @@ -536,7 +536,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
return XCTFail("Expected to still have connections available")
}

let mockRequest = MockHTTPRequest(eventLoop: eventLoop)
let mockRequest = MockHTTPScheduableRequest(eventLoop: eventLoop)
let request = HTTPConnectionPool.Request(mockRequest)
let action = state.executeRequest(request)

Expand All @@ -553,7 +553,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
for _ in 0..<100 {
let eventLoop = elg.next()

let mockRequest = MockHTTPRequest(eventLoop: eventLoop, requiresEventLoopForChannel: false)
let mockRequest = MockHTTPScheduableRequest(eventLoop: eventLoop, requiresEventLoopForChannel: false)
let request = HTTPConnectionPool.Request(mockRequest)
let action = state.executeRequest(request)

Expand Down Expand Up @@ -667,7 +667,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
retryConnectionEstablishment: true
)

let mockRequest = MockHTTPRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false)
let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false)
let request = HTTPConnectionPool.Request(mockRequest)

let executeAction = state.executeRequest(request)
Expand Down Expand Up @@ -706,7 +706,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
retryConnectionEstablishment: true
)

let mockRequest = MockHTTPRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false)
let mockRequest = MockHTTPScheduableRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false)
let request = HTTPConnectionPool.Request(mockRequest)

let executeAction = state.executeRequest(request)
Expand Down Expand Up @@ -738,7 +738,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
retryConnectionEstablishment: true
)

let mockRequest = MockHTTPRequest(eventLoop: eventLoop.next(), requiresEventLoopForChannel: false)
let mockRequest = MockHTTPScheduableRequest(eventLoop: eventLoop.next(), requiresEventLoopForChannel: false)
let request = HTTPConnectionPool.Request(mockRequest)

let executeAction = state.executeRequest(request)
Expand All @@ -762,7 +762,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
retryConnectionEstablishment: true
)

let mockRequest1 = MockHTTPRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false)
let mockRequest1 = MockHTTPScheduableRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false)
let request1 = HTTPConnectionPool.Request(mockRequest1)

let executeAction1 = state.executeRequest(request1)
Expand All @@ -773,7 +773,7 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {

XCTAssertEqual(executeAction1.request, .scheduleRequestTimeout(for: request1, on: mockRequest1.eventLoop))

let mockRequest2 = MockHTTPRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false)
let mockRequest2 = MockHTTPScheduableRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false)
let request2 = HTTPConnectionPool.Request(mockRequest2)

let executeAction2 = state.executeRequest(request2)
Expand Down
Loading