-
Notifications
You must be signed in to change notification settings - Fork 419
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
Mitigate zero-length writes issue. #917
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, one small test suggestion.
|
||
func noZeroLengthWriteExpectation() -> XCTestExpectation { | ||
let expectation = self.expectation(description: "Not expecting zero length write workaround") | ||
expectation.isInverted = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than inverting the expectation, can we fulfil our expectation when we see the error from not finding the handler instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do, but it breaks the test symmetry: I have to write two test flows instead. Is that your preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why two test flows? Couldn't we just pass two (Result<NIOFilterEmptyWritesHandler, Error>) -> Void
closures through to _runTest
and debugPipelineExpectation
and run them run in an always
block rather than passing through the expectation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yeah, we can do that. It's a bit less clean at the test site, but works fine.
Motivation: On some iOS releases, zero-length writes cause problems for Network.framework. These problems can lead to difficult to debug catastrophic failure modes, so they ought to be avoided. NIOTransportServices provides a workaround channel handler, which we ought to use. Modifications: - Add a PlatformSupport check for whether to apply the workaround. - Add the workaround code to the pipeline configuration callbacks. - Tests Results: Users can work around the zero length writes problem.
4fe97e8
to
cbc422e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
Motivation:
On some iOS releases, zero-length writes cause problems for
Network.framework. These problems can lead to difficult to debug
catastrophic failure modes, so they ought to be avoided.
NIOTransportServices provides a workaround channel handler, which we
ought to use.
Modifications:
Results:
Users can work around the zero length writes problem.