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
Added latch to async SyncOnSubscrbeTest #3285
Conversation
#3286 has encountered another failure:
|
c7f8178
to
d359e6a
Compare
This should also resolve #3287. I have added your example test to the unit tests (3,000 iterations). |
IS_UNSUBSCRIBED.compareAndSet(this, 0, 1); | ||
if (get() == 0L) | ||
parent.onUnsubscribe(state); | ||
while(true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a much simpler way of fixing this: undo all these latest changes and request 1!
if (BackpressureUtils.getAndAddRequest(this, 1) == 0) {
parent.onUnsubscribe(state);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's interesting. If there is currently a thread processing over the request count loop (i.e. processing nextIteration()) then won't this have to block the unsubscribing thread? The unsubscribing thread would have to poll over this until it got access. I'm not seeing how this would force a thread currently iterating to terminate. If you read through the concurrency I think you will see that it's not really all that complicated and I believe it sufficiently covers the possible race conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no blocking. My suggestion follows the queue-drain approach where you queue the terminal flag and drain it in the emission loop. Since the loop first checks for the terminal flag there won't be any wrong emissions and the loop quits. If unsubscribe increments from zero, there isn't and won't be any emission loop after that and the resource can be freed.
There is no need for your complicated solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing where you are suggesting setting a terminal flag. You are more than welcome to open your own pull request to demonstrate your fix. My pull request does set a flag for the draining thread to signal when it needs to unsubscribe.
d359e6a
to
1d6657d
Compare
1d6657d
to
583222d
Compare
👍 |
Okay, let's merge this so other PRs don't run into the test bug with the current main. |
Added latch to async SyncOnSubscrbeTest
No description provided.