Skip to content

BaseStreamSocketChannel half-close allows outstanding writes to complete #3148

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
16 changes: 12 additions & 4 deletions Sources/NIOPosix/BaseStreamSocketChannel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -194,11 +194,19 @@ class BaseStreamSocketChannel<Socket: SocketProtocol>: BaseSocketChannel<Socket>
self.close0(error: error, mode: .all, promise: promise)
return
}
try self.shutdownSocket(mode: mode)
// Fail all pending writes and so ensure all pending promises are notified
self.pendingWrites.failAll(error: error, close: false)
self.unregisterForWritable()
promise?.succeed(())

// Shutdown the socket only when the pending writes are dealt with
let closePromise = self.eventLoop.makePromise(of: Void.self)
closePromise.futureResult.whenComplete { _ in
do {
try self.shutdownSocket(mode: mode)
promise?.succeed(())
} catch let err {
promise?.fail(err)
}
}
self.pendingWrites.close(closePromise)

self.pipeline.fireUserInboundEventTriggered(ChannelEvent.outputClosed)
Copy link
Contributor

Choose a reason for hiding this comment

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

This user event now triggers unconditionally, and at the wrong time. It should trigger only after the shutdown. Also, don't forget the unregister for writable

case .input:
Expand Down
11 changes: 7 additions & 4 deletions Sources/NIOPosix/PendingDatagramWritesManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,8 @@ private struct PendingDatagramWritesState {
// When we've hit an error we treat it like fully writing the first datagram. We aren't going to try to
// send it again.
let promiseFiller = self.wroteFirst(error: error)
let result: OneWriteOperationResult = self.pendingWrites.hasMark ? .writtenPartially : .writtenCompletely
let result: OneWriteOperationResult =
self.pendingWrites.hasMark ? .writtenPartially : .writtenCompletely(closePromise: nil)

return (promiseFiller, result)
}
Expand Down Expand Up @@ -305,7 +306,8 @@ private struct PendingDatagramWritesState {
}

// If we no longer have a mark, we wrote everything.
let result: OneWriteOperationResult = self.pendingWrites.hasMark ? .writtenPartially : .writtenCompletely
let result: OneWriteOperationResult =
self.pendingWrites.hasMark ? .writtenPartially : .writtenCompletely(closePromise: nil)
return (promiseFiller, result)
}

Expand All @@ -322,7 +324,8 @@ private struct PendingDatagramWritesState {
)
let writeFiller = self.wroteFirst()
// If we no longer have a mark, we wrote everything.
let result: OneWriteOperationResult = self.pendingWrites.hasMark ? .writtenPartially : .writtenCompletely
let result: OneWriteOperationResult =
self.pendingWrites.hasMark ? .writtenPartially : .writtenCompletely(closePromise: nil)
return (writeFiller, result)
}

Expand Down Expand Up @@ -565,7 +568,7 @@ final class PendingDatagramWritesManager: PendingWritesManager {
preconditionFailure("PendingDatagramWritesManager was handed a file write")
case .nothingToBeWritten:
assertionFailure("called \(#function) with nothing available to be written")
return OneWriteOperationResult.writtenCompletely
return OneWriteOperationResult.writtenCompletely(closePromise: nil)
}
}
}
Expand Down
Loading
Loading