Refactor provider shutdown and pending flows #240
Conversation
artemredkin
changed the title
| let provider = HTTP1ConnectionProvider(key: key, | ||
| eventLoop: taskEventLoop, | ||
| configuration: self.configuration, | ||
| pool: self, | ||
| backgroundActivityLogger: self.backgroundActivityLogger) | ||
| _ = provider.enqueue() |
There was a problem hiding this comment.
yippie! Could we assert here that the return value is the expected one?
There was a problem hiding this comment.
great idea, done!
| @@ -64,7 +64,7 @@ extension HTTP1ConnectionProvider { | |||
| private var openedConnectionsCount: Int = 0 | |||
|
|
|||
| /// Number of enqueued requests, used to track if it is safe to delete the provider. | |||
| private var pending: Int = 1 | |||
| private var pending: Int = 0 | |||
There was a problem hiding this comment.
nice! I left a bunch of stuff like the following comments in the code
/* BEGIN workaround for #232, this whole block is to be replaced by the commented out line below */
// not closing the provider here (https://github.com/swift-server/async-http-client/issues/232)
var state = self.http1ConnectionProvider.state.testsOnly_getInternalState()
if state.pending == 1, state.waiters.isEmpty, state.leasedConnections.isEmpty, state.openedConnectionsCount == 0 {
state.pending = 0
self.http1ConnectionProvider.state.testsOnly_setInternalState(state)
}
self.http1ConnectionProvider.closePromise.succeed(())
/* END workaround for #232 */
and
// cleanup
// this cleanup code needs to go and use HTTP1ConnectionProvider's API instead
I believe we can also delete all this code then.
There was a problem hiding this comment.
I was able to delete the workaround, some clean up code, and I was able to replace some cleanup with API calls instead of direct state manipulation. Currently there are 3-4 tests that manipulate state directly (they all need the state to be near the limit), do you want me to fix it here or when I get to #232?
There was a problem hiding this comment.
no, let's fix them in a followup. It's super awesome you could remove the workaround
| @@ -148,7 +148,7 @@ extension HTTP1ConnectionProvider { | |||
| return .none | |||
| } | |||
| case .closed: | |||
| return .fail(waiter, ProviderClosedError()) | |||
| return .fail(waiter, HTTPClientError.alreadyShutdown) | |||
There was a problem hiding this comment.
@artemredkin from what I can tell, no test noticed that this error changed which probably means no test is getting this error ;). I think we should probably add a test case that tests this condition.
There was a problem hiding this comment.
Specific test is a good idea, yes, though this error was picked up by a test, just not reliably :)
Fixes some edge cases in shutdown and pending states.
Motivation:
There are two issues this PR addresses:
enqueueand get an unexpected error+1pending state, while it might not be an error, it makes testing difficult, also might lead to races in shutdown.Modifications:
HTTP1ConnectionsProvideris no longer created with+1pendingclosewhile request is enqueued, it should be failed with a more expected.alreadyShutdownerror, since its the only way to get into that state, so test was actually catching the right thing.Result:
Closes #226
Closes #323