Skip to content
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

fix test and a crash when closed #254

Merged

Conversation

artemredkin
Copy link
Collaborator

Fixes a crash in testUncleanCloseThrows.

Motivation:
This PR aims to fix two issues:

  1. A crash on precondition, that was not removed when I re-factored how shutdowns work, it is absolutely normal to get a connection failure when client is shutting down, for example in the following scenario:
  • send a request that will fail to connect
  • shutdown the client
  • when connect error will be processed, client will be .closed, so it's a completely normal situatuon
  1. A racy test, that test tries to establish a connection and shutdown the client while this connection is alive. But it does so in a racy fashion, there is no guarantee that when we get to client shutdown, connection has been established. This will be addressed by using NIOHTTP1TestServer, where we wait for .head before we attempt to shutdown the client.

Modifications:

  1. Rewritten the testUncleanCloseThrows to use NIOHTTP1TestServer to eliminate a race
  2. Removed precondition in ConnectionState.swift
  3. Added a test to make sure that connectFailed is working as expected when the client is .closed

Result:
Closes #248

@artemredkin artemredkin requested a review from weissi June 15, 2020 09:46
@artemredkin artemredkin added this to the 1.2.0 milestone Jun 15, 2020
var state = HTTP1ConnectionProvider.ConnectionsState(eventLoop: self.eventLoop)
var snapshot = state.testsOnly_getInternalState()
snapshot.state = .closed
state.testsOnly_setInternalState(snapshot)
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we achieve this state by driving the state machine rather than forcing in a possibly illegal state?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah, good catch, we can indeed, done, thanks! (also fixed a similar test above)

Copy link
Contributor

@weissi weissi left a comment

Choose a reason for hiding this comment

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

Awesome, thanks, LGTM!

@artemredkin artemredkin merged commit a61b31c into swift-server:master Jun 15, 2020
@artemredkin artemredkin deleted the fix_unclean_shutdown_test branch June 15, 2020 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

testUncleanShutdown crashes if shutdown is executed before the request
2 participants