Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Deliver HTTPServerUpgradeHandler data on removal. #1092

Merged

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Aug 5, 2019

Motivation:

There is currently a pair of windows in which it is possible for the
HTTPServerUpgradeHandler to buffer some data without replaying it. This
can happen if it is reentrantly called just after it finishes
unbuffering data, which is not great.

To avoid this, we move to execute this data delivery while we're
removing ourselves. This ensures that we are confident that we cannot be
reentrantly called after our last outcall, meaning we can be confident
of 100% data delivery.

Modifications:

  • Moved delayed data delivery to removeHandler.

Result:

Less chance of quiet data loss

Motivation:

There is currently a pair of windows in which it is possible for the
HTTPServerUpgradeHandler to buffer some data without replaying it. This
can happen if it is reentrantly called just after it finishes
unbuffering data, which is not great.

To avoid this, we move to execute this data delivery while we're
removing ourselves. This ensures that we are confident that we cannot be
reentrantly called after our last outcall, meaning we can be confident
of 100% data delivery.

Modifications:

- Moved delayed data delivery to removeHandler.

Result:

Less chance of quiet data loss
@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Aug 5, 2019
@Lukasa Lukasa added this to the 2.7.0 milestone Aug 5, 2019
@Lukasa Lukasa requested a review from weissi August 5, 2019 13:40
Copy link
Member

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

Fantastic, thank you!

@weissi weissi merged commit 5d54458 into apple:master Aug 5, 2019
@Lukasa Lukasa modified the milestones: 2.7.0, 2.6.1 Aug 9, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants