Improve errors and testing using NIOTS #588
Conversation
Lukasa
added
the
semver/minor
Motivation Currently error reporting with NIO Transport Services is often sub-par. This occurs because the Network.framework connections may enter the waiting state until the network connectivity state changes. We were not watching for the user event that contains the error in that state, so if we timed out in that state we'd just give a generic timeout error, instead of telling the user anything more detailed. Additionally, several of our tests assume that failure will be fast, but in NIO Transport Services we will enter that .waiting state. This is reasonable, as changed network connections may make a connection that was not succeeding suddenly viable. However, it's inconvenient for testing, where we're mostly interested in confirming that the error path works as expected. Modifications - Add an observer of the WaitingForConnectivity event that records it into our state machine for later reporting. - Add support for disabling waiting for connectivity for testing purposes. - Add annotations to several tests to stop them waiting for connectivity. Results Faster tests, better coverage, better errors for our users.
Sources/AsyncHTTPClient/NIOTransportServices/NWWaitingHandler.swift
Outdated
Show resolved
Hide resolved
Co-authored-by: David Nadoba <dnadoba@gmail.com>
There was a problem hiding this comment.
This looks great. And I'm sure that the new code path is hit plenty in more complex scenarios. However I would love to see two more test cases:
- Unit style: the new
NWWaitingHandlerin anEmbeddedChannel. Is the callout triggered. - An explicit test: NWError is reported if
networkFrameworkWaitForConnectivityis turned on. Currently this is implicit intestAvoidLeakingTLSHandshakeCompletionPromise.
Doing those in a follow up is totally fine with me.
| } | ||
|
|
||
| func handlerRemoved(context: ChannelHandlerContext) { | ||
| self.requester = nil |
There was a problem hiding this comment.
just out of interest, why do we release the requester here explicitly? Could there be a race that we get an inbound event after we have been removed from the pipeline?
There was a problem hiding this comment.
No, this was initially defensive to prevent requesters living an excessively long time. However, now that I think about it I suspect this is unnecessarily defensive, as nothing else is holding onto this handler.
|
I've addressed (2), I'm going to tackle (1) in the future because it requires me to make an enhancement to NIOTS. |
|
5.7/nightly failures are Sendable related, merging over them. |
Motivation
Currently error reporting with NIO Transport Services is often sub-par.
This occurs because the Network.framework connections may enter the
waiting state until the network connectivity state changes. We were not
watching for the user event that contains the error in that state, so if
we timed out in that state we'd just give a generic timeout error,
instead of telling the user anything more detailed.
Additionally, several of our tests assume that failure will be fast, but
in NIO Transport Services we will enter that .waiting state. This is
reasonable, as changed network connections may make a connection that
was not succeeding suddenly viable. However, it's inconvenient for
testing, where we're mostly interested in confirming that the error path
works as expected.
Modifications
into our state machine for later reporting.
purposes.
connectivity.
Results
Faster tests, better coverage, better errors for our users.