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 NIOAsyncSequenceProducer watermark strategy. #2952

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

FranzBusch
Copy link
Member

Motivation

It was currently possible that the producer's delegate is getting called twice with produceMore even if no yield returned a stopProducing. This could happen when we expected the producer to yield elements but the consumer went below the low watermark again. Resulting in two subsequent calls.

Modification

This PR stores the current demand state in the strategy which let's us avoid flipping the hasOustandingDemand state of the sequence.

Result

Correctly, ensured the call order of produceMore and stopProducing.

@FranzBusch FranzBusch force-pushed the fb-async-sequence-producer-fix branch 3 times, most recently from 4aba8e4 to 1fe8490 Compare October 28, 2024 14:14
@FranzBusch FranzBusch added the 🔨 semver/patch No public API change. label Oct 28, 2024
@FranzBusch FranzBusch marked this pull request as ready for review October 28, 2024 14:15
# Motivation

It was currently possible that the producer's delegate is getting called twice with `produceMore` even if no `yield` returned a `stopProducing`. This could happen when we expected the producer to yield elements but the consumer went below the low watermark again. Resulting in two subsequent calls.

# Modification

This PR stores the current demand state in the strategy which let's us avoid flipping the `hasOustandingDemand` state of the sequence.

# Result

Correctly, ensured the call order of `produceMore` and `stopProducing`.
@FranzBusch FranzBusch force-pushed the fb-async-sequence-producer-fix branch from 1fe8490 to ff1408c Compare October 28, 2024 14:20
@FranzBusch FranzBusch requested a review from glbrntt October 28, 2024 14:48
Copy link
Contributor

@rnro rnro left a comment

Choose a reason for hiding this comment

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

Looks good!

@FranzBusch FranzBusch merged commit 02906a6 into main Oct 28, 2024
41 of 43 checks passed
@FranzBusch FranzBusch deleted the fb-async-sequence-producer-fix branch October 28, 2024 15:14
@IgorMuzyka
Copy link

@rnro hi, not sure who to mention here but you approved changes here so maybe it is you. My question is how soon a new release with this changes merged can be expected?

@Lukasa
Copy link
Contributor

Lukasa commented Nov 8, 2024

We are unlikely to release before next week at the absolute earliest. However, if we released today our main branch would break Hummingbird (and a few other packages) because of swiftlang/swift#77459. So we're waiting to see what the path of that patch is going to be.

@IgorMuzyka
Copy link

@Lukasa thanks, thats an unexpected (for me) turn, but next week as an optimistic release date is great nevertheless. Won't pretend to understand fully the issue you mentioned, but since it concerns those who use swift 6, can't the changes in this issue be wrapped with #if swift(<=6.0) ?

@Lukasa
Copy link
Contributor

Lukasa commented Nov 8, 2024

This issue isn't the one that's affected, the issue is a number of other fixes that have landed in main as well.

If we aren't going to get a fix to that issue in a timely fashion then we'll need to back those fixes out.

@Volodymyr-13
Copy link

@Lukasa Hi Cory, can you give me an update on the release date for this? My app crashes a lot because of this, using Vapor, and they are waiting for the release of NIO. It's been a long wait! Waiting -> Waiting)

@Lukasa
Copy link
Contributor

Lukasa commented Nov 20, 2024

Hello! I'll aim to get you a release this week or next: we'll have to back some stuff out to work around the linked bug.

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.

5 participants