-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 for Infinite loop in PersistentReplicator.startProducer() #275
Conversation
First commit solved the immediate problem - 072fe7e Second commit restructures the code to solve both the immediate problem and the problem @rdhabalia had tried to fix - c1088ac4a2377a535f2503ffee2fd17331d3e1d9 |
can you please add the unit-test case where it tries to disconnect twice. first time it disconnects which stops producer, second time it should not flip the state and try to again restart the replicator which should restart it successfully. |
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.
let's fix this one and we can put it in the patch.
|| STATE_UPDATER.compareAndSet(this, State.Started, State.Stopping))) { | ||
if (producer == null) { | ||
// If there's already a reconnection happening, signal to close it whenever it's ready | ||
STATE_UPDATER.compareAndSet(this, State.Stopped, State.Stopped); |
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.
here essentially, we are setting the state Stopped
if it's already Stopped
. I don't think we need this here.
} else { | ||
// If there's already a reconnection happening, signal to close it whenever it's ready | ||
STATE_UPDATER.set(this, State.Stopping); | ||
} else if (STATE_UPDATER.get(this) == State.Stopping) { |
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.
do we really need this else block because we actually don't do anything.
May be we can check in the beginning that if state is Stopping
then return CompletableFuture.completedFuture(null)
@rdhabalia @saandrews - added test case |
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.
👍
* Fix for Infinite loop in PersistentReplicator.startProducer() * Fix for Infinite loop in PersistentReplicator.startProducer()
Added microbenchmark to report compression and decompression speeds.
…pache#275) (cherry picked from commit 6a2a010)
…pache#275) (cherry picked from commit 6a2a010)
…pache#275) (cherry picked from commit 6a2a010)
…pache#275) (cherry picked from commit 6a2a010)
Motivation
If replicator is stopped twice the state changes to "Stopping" and then when you start producer it goes into infinite loop. since there is a condition that checks if state is Stopping
Bug in #232
Modifications
Changed the logic and added comments for reasoning
Result
No inifinite loops in replication logic