Skip to content

Conversation

@Lukasa
Copy link
Collaborator

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.
@LukasaLukasa added the 🔨 semver/patch No public API change. label Mar 15, 2021
@LukasaLukasa requested a review from weissiMarch 15, 2021 13:30
@Lukasa
Copy link
CollaboratorAuthor

@swift-server-bot test this please

@Lukasa
Copy link
CollaboratorAuthor

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

@Lukasa
Copy link
CollaboratorAuthor

@swift-server-bot test this please

@Lukasa
Copy link
CollaboratorAuthor

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

@Lukasa
Copy link
CollaboratorAuthor

@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
CollaboratorAuthor

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

@LukasaLukasa merged commit ae5f185 into swift-server:mainMar 16, 2021
@LukasaLukasa deleted the cb-synchronous-pipeline-ops branch March 16, 2021 09:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔨 semver/patchNo public API change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Lukasa@artemredkin