Skip to content

Handle channel closure properly #20

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.

Sign up for GitHub

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

Merged
gjcairo merged 1 commit into from
Oct 23, 2025
Merged

Handle channel closure properly #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 1 addition & 3 deletions Sources/HTTPServer/HTTPResponseConcludingAsyncWriter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,7 @@ public struct HTTPResponseConcludingAsyncWriter: ConcludingAsyncWriter, ~Copyabl
let responseBodyAsyncWriter = ResponseBodyAsyncWriter(writer: self.writer)
let (result, finalElement) = try await body(responseBodyAsyncWriter)
try await self.writer.write(.end(finalElement))
self.writerState.wrapped.withLock { state in
state.finishedWriting = true
}
self.writerState.wrapped.withLock { $0.finishedWriting = true }
return result
}
}
Expand Down
28 changes: 17 additions & 11 deletions Sources/HTTPServer/HTTPServer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ public final class Server<RequestHandler: HTTPServerRequestHandler> {
try await serverChannel.executeThenClose { inbound in
for try await http1Channel in inbound {
group.addTask {
await Self.handleRequestChannel(
try await Self.handleRequestChannel(
logger: logger,
channel: http1Channel,
handler: handler
Expand Down Expand Up @@ -302,7 +302,7 @@ public final class Server<RequestHandler: HTTPServerRequestHandler> {
switch try await upgradeResult.get() {
case .http1_1(let http1Channel):
connectionGroup.addTask {
await Self.handleRequestChannel(
try await Self.handleRequestChannel(
logger: logger,
channel: http1Channel,
handler: handler
Expand All @@ -312,7 +312,7 @@ public final class Server<RequestHandler: HTTPServerRequestHandler> {
do {
for try await http2StreamChannel in http2Multiplexer.inbound {
connectionGroup.addTask {
await Self.handleRequestChannel(
try await Self.handleRequestChannel(
logger: logger,
channel: http2StreamChannel,
handler: handler
Expand All @@ -338,7 +338,7 @@ public final class Server<RequestHandler: HTTPServerRequestHandler> {
logger: Logger,
channel: NIOAsyncChannel<HTTPRequestPart, HTTPResponsePart>,
handler: RequestHandler
) async {
) async throws {
do {
try await channel
.executeThenClose { inbound, outbound in
Expand Down Expand Up @@ -380,17 +380,17 @@ public final class Server<RequestHandler: HTTPServerRequestHandler> {
}
)
} catch {
logger.error("Error thrown while handling connection: \(error)")
Copy link
Collaborator

@Lukasa Lukasa Oct 23, 2025

Choose a reason for hiding this comment

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

Prefer using metadata to emit the error.

Copy link
Collaborator Author

@gjcairo gjcairo Oct 23, 2025

Choose a reason for hiding this comment

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

Yes, it's on my radar - I need to change this everywhere.

if !readerState.wrapped.withLock({ $0.finishedReading }) {
// TODO: do something - we didn't finish reading but we threw
// if h2 reset stream; if h1 try draining request?
fatalError("Didn't finish reading but threw.")
logger.error("Did not finish reading but error thrown.")
// TODO: if h2 reset stream; if h1 try draining request?
}
if !writerState.wrapped.withLock({ $0.finishedWriting }) {
// TODO: this means we didn't write a response end and we threw
// we need to do something, possibly just close the connection or
logger.error("Did not write response but error thrown.")
// TODO: we need to do something, possibly just close the connection or
// reset the stream with the appropriate error.
fatalError("Didn't finish writing but threw.")
}
throw error
}

// TODO: handle other state scenarios.
Expand All @@ -399,10 +399,16 @@ public final class Server<RequestHandler: HTTPServerRequestHandler> {
// If we finished reading but we didn't write back a response, then RST_STREAM
// is also likely appropriate but unclear about the error.
// For h1, we should close the connection.

// Finish the outbound and wait on the close future to make sure all pending
// writes are actually written.
outbound.finish()
try await channel.channel.closeFuture.get()
}
} catch {
logger.debug("Error thrown while handling connection: \(error)")
logger.error("Error thrown while handling connection: \(error)")
// TODO: We need to send a response head here potentially
throw error
}
}
}
Expand Down
Loading