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

HappyEyeballs should tolerate uncancellable tasks. #191

Merged
merged 1 commit into from
Mar 20, 2018

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Mar 19, 2018

Motivation:

The HappyEyeballs state machine incorrectly assumed that it would always
be able to cancel any scheduled task that would execute on the event loop
that the state machine itself was running on. This is not accurate: if
the scheduled task is scheduled to execute at roughly the same time as
another callback into the state machine then it may be uncancellable.

Modifications:

Allowed the three scheduled tasks (connect timeout, connect delay, and
resolver delay) to fire in completed.

Result:

No fatalErrors in window conditions under high load

Resolves #187.

@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Mar 19, 2018
@Lukasa Lukasa added this to the 1.3.0 milestone Mar 19, 2018
@Lukasa Lukasa requested review from normanmaurer and weissi March 19, 2018 12:44
channels.finishAll()
}

let (eyeballer, resolver, loop) = buildEyeballer(host: "example.com", port: 80, connectTimeout: .milliseconds(250)) {
Copy link
Member

Choose a reason for hiding this comment

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

@Lukasa maybe we just want to use swiftnio.io for the hostname everywhere tho ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it doesn't really matter?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah sure it not matters in this test but if we have a domain name we can also just use it and not refer to some random ones. Feel free to ignore tho, I don’t say we need to do it but I think knit is a good thing in general

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, an example.com is just OK for this purpose, see rfc2606.

Copy link
Member

Choose a reason for hiding this comment

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

@vlm TIL ... Thanks!

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.

👍 makes sense!

@normanmaurer
Copy link
Member

@Lukasa can you rebase and merge ?

Motivation:

The HappyEyeballs state machine incorrectly assumed that it would always
be able to cancel any scheduled task that would execute on the event loop
that the state machine itself was running on. This is not accurate: if
the scheduled task is scheduled to execute at roughly the same time as
another callback into the state machine then it may be uncancellable.

Modifications:

Allowed the three scheduled tasks (connect timeout, connect delay, and
resolver delay) to fire in completed.

Result:

No fatalErrors in window conditions under high load
@Lukasa Lukasa force-pushed the cb-handle-cancellation-of-delayed-tasks branch from 20a1dbd to 02db81e Compare March 20, 2018 14:59
@Lukasa
Copy link
Contributor Author

Lukasa commented Mar 20, 2018

@swift-nio-bot test this please

@Lukasa Lukasa merged commit fef728c into apple:master Mar 20, 2018
@Lukasa Lukasa deleted the cb-handle-cancellation-of-delayed-tasks branch March 20, 2018 16:14
weissi pushed a commit to weissi/swift-nio that referenced this pull request Jun 13, 2020
Motivation:

When a window update event is received in the stream channel we notify
the inbound window manager. Doing so may emit a WINDOW_UPDATE frame
to indicate the remote may send us more DATA. However, this happens even
if there are inbound frames waiting to be delivered to the channel. This
may lead to the remote sending us more DATA which we have to buffer
until it is read.

Modifications:

- Update the inbound window manager to track the number of bytes
  buffered in the stream channel
- Account for buffered bytes when receiving a new window size event
- Possibly emit WINDOW_UPDATE after unbuffering inbound frames into the
  stream channel's pipeline

Result:

The timing of WINDOW_UPDATE frames is better aligned with when frames
are actually read in a stream.
weissi pushed a commit to weissi/swift-nio that referenced this pull request Feb 3, 2024
Motivation:

Docs are generated by and hosted on the Swift Package Index. We no
longer need the script to generate docs via Jazzy.

Modifications:

- Remove the generate_docs script
- Remove Jazzy from the Dockerfile but keep Ruby; it's used for
  generating test manifests.
- Remove SwiftFormat from the Dockerfile; we don't use it.

Result:

Fewer unused things.
# 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.

fatalError() during high concurrency test
4 participants