Skip to content

Use synchronous pipeline hops to remove windows. #346

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

Merged
merged 1 commit into from
Mar 16, 2021

Conversation

Lukasa
Copy link
Collaborator

@Lukasa Lukasa commented Mar 15, 2021

Motivation:

There is an awkward timing window in the TLSEventsHandler flow where it
is possible for the NIOSSLClientHandler to fail the handshake on
handlerAdded. If this happens, the TLSEventsHandler will not be in the
pipeline, and so the handshake failure error will be lost and we'll get
a generic one instead.

This window can be resolved without performance penalty if we use the
new synchronous pipeline operations view to add the two handlers
backwards. If this is done then we can ensure that the TLSEventsHandler
is always in the pipeline before the NIOSSLClientHandler, and so there
is no risk of event loss.

While I'm here, AHC does a lot of pipeline modification. This has led to
lengthy future chains with lots of event loop hops for no particularly
good reason. I've therefore replaced all pipeline operations with their
synchronous counterparts. All but one sequence was happening on the
correct event loop, and for the one that may not I've added a fast-path
dispatch that should tolerate being on the wrong one. The result is
cleaner, more linear code that also reduces the allocations and event
loop hops.

Modifications:

  • Use synchronous pipeline operations everywhere
  • Change the order of adding TLSEventsHandler and NIOSSLClientHandler

Result:

Faster, safer, fewer timing windows.

Motivation:

There is an awkward timing window in the TLSEventsHandler flow where it
is possible for the NIOSSLClientHandler to fail the handshake on
handlerAdded. If this happens, the TLSEventsHandler will not be in the
pipeline, and so the handshake failure error will be lost and we'll get
a generic one instead.

This window can be resolved without performance penalty if we use the
new synchronous pipeline operations view to add the two handlers
backwards. If this is done then we can ensure that the TLSEventsHandler
is always in the pipeline before the NIOSSLClientHandler, and so there
is no risk of event loss.

While I'm here, AHC does a lot of pipeline modification. This has led to
lengthy future chains with lots of event loop hops for no particularly
good reason. I've therefore replaced all pipeline operations with their
synchronous counterparts. All but one sequence was happening on the
correct event loop, and for the one that may not I've added a fast-path
dispatch that should tolerate being on the wrong one. The result is
cleaner, more linear code that also reduces the allocations and event
loop hops.

Modifications:

- Use synchronous pipeline operations everywhere
- Change the order of adding TLSEventsHandler and NIOSSLClientHandler

Result:

Faster, safer, fewer timing windows.
@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Mar 15, 2021
@Lukasa Lukasa requested a review from weissi March 15, 2021 13:30
@Lukasa
Copy link
Collaborator Author

Lukasa commented Mar 15, 2021

@swift-server-bot test this please

@Lukasa
Copy link
Collaborator Author

Lukasa commented Mar 15, 2021

CI is busted, we don't break the API here.

@Lukasa
Copy link
Collaborator Author

Lukasa commented Mar 16, 2021

@swift-server-bot test this please

@Lukasa
Copy link
Collaborator Author

Lukasa commented Mar 16, 2021

Flaky test, tracking in #347, re-running.

@Lukasa
Copy link
Collaborator Author

Lukasa commented Mar 16, 2021

@swift-server-bot test this please

@artemredkin
Copy link
Collaborator

There are other places were we use pipeline modifications (in the connection pool we add/remove idle and task handlers), would they benefit from sync operations as well?

@Lukasa
Copy link
Collaborator Author

Lukasa commented Mar 16, 2021

Probably, but in this instance I wanted to just tackle the ones that I could see were clearly worthwhile.

@Lukasa Lukasa merged commit ae5f185 into swift-server:main Mar 16, 2021
@Lukasa Lukasa deleted the cb-synchronous-pipeline-ops branch March 16, 2021 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants