Propagate URLSession errors to HTTPBodyOutputStreamBridge - #94
Conversation
| deinit { | ||
| debug("Output stream delegate deinit") | ||
| outputStream.delegate = nil | ||
| if outputStream.streamStatus != .notOpen && outputStream.streamStatus != .closed { |
There was a problem hiding this comment.
Added this while debugging. Shouldn't really be necessary any more with the other changes but I think it's a good defensive check
There was a problem hiding this comment.
Most NSStream APIs are not thread-safe but deinit can run on any thread, so it is not safe to call its methods here.
There was a problem hiding this comment.
Will just remove. Will have to drop the unit test for this too.
| deinit { | ||
| debug("Output stream delegate deinit") | ||
| outputStream.delegate = nil | ||
| if outputStream.streamStatus != .notOpen && outputStream.streamStatus != .closed { |
There was a problem hiding this comment.
Most NSStream APIs are not thread-safe but deinit can run on any thread, so it is not safe to call its methods here.
|
@guoye-zhang Updated per your feedback |
| mutating func errorOccurred(_ error: any Error) -> Action { | ||
| switch self { | ||
| case .initial: | ||
| self = .closed(error) |
There was a problem hiding this comment.
Is this change necessary?
There was a problem hiding this comment.
I believe it is. In the initializer, the output stream is opened and the state is set to initial. If URLSession receives an error before the state updates then the output stream is never closed.
### Motivation #94 added a new `cancel()` path used by the `URLSession` delegate to clean up `HTTPBodyOutputStream` resources on errors received by the delegate but not forwarded to the output stream. As detailed in apple/swift-openapi-generator#918 though, we need to refactor the existing code so that late-arriving terminal events (`endEncountered`, `wroteFinalChunk` and `errorOccurred`) from the CFStream delegate and the writer tasks do not trigger a fatal `preconditionFailure()` if an error was already forwarded by the `URLSession` delegate via `cancel()`. ### Modifications Emit a `debug()` log and return an action of `.none` if the stream is in a `closed` state when handling an `endEncountered`, `wroteFinalChunk` or `errorOccurred` event. ### Result There is no longer a race condition that can lead to a fatal `preconditionFailure`. ### Test Plan Added unit tests to cover specific even handling changes.
2 participants
Motivation
Resolves leaks described in swift-openapi-generator#917 by explicitly passing
URLSessionerrors on toHTTPBodyOutputStreamBridgefor proper cleanup.Modifications
Added a
cancel(error)method toHTTPBodyOutputStreamBridgewhichBidirectionalStreamingURLSessionDelegatecalls directly when handling aURLSessionerror.Result
The
outputStreamis closed onURLSessionerrors and the writerTaskis cancelled if it was running to avoid keeping a strong reference toself.Test Plan