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 deadlock reported by #426 when FIFORunnable is disposed before running #428

Merged
merged 4 commits into from
May 24, 2018

Conversation

bleeding182
Copy link
Contributor

@bleeding182 bleeding182 commented May 18, 2018

As reported we should release the semaphore when we don't execute the operation.
Fixes #426

@CLAassistant
Copy link

CLAassistant commented May 18, 2018

CLA assistant check
All committers have signed the CLA.

@dariuszseweryn
Copy link
Owner

@bleeding182 Did you confirmed that this change indeed helps?

@bleeding182
Copy link
Contributor Author

@DariuszAniszewski Hey there!

Yes, been debugging today, can't reproduce it with this fix.

@dariuszseweryn
Copy link
Owner

It's @dariuszseweryn ;)

Great to hear that. I think that we should add a test for FIFORunnableEntry to prevent such a situation in the future.

The test should be straightforward to write and just verify that the semaphore is either released or passed to an operation depending on ObservableEmitter.isDisposed() result.

P.S. Maybe it would be a bit clearer if the code be something like:

if (observableEmitter.isDisposed()) {
  RxBleLog...
  semaphore.release();
  return;
}
operation.run(semaphore)...

@dariuszseweryn dariuszseweryn requested a review from uKL May 22, 2018 15:24
@dariuszseweryn dariuszseweryn self-assigned this May 22, 2018
Copy link
Collaborator

@uKL uKL left a comment

Choose a reason for hiding this comment

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

One grammar suggestion.

@@ -35,44 +35,46 @@ public int compareTo(@NonNull FIFORunnableEntry other) {
}

public void run(QueueSemaphore semaphore, Scheduler subscribeScheduler) {

if (operationResultObserver.isDisposed()) {
RxBleLog.d("Operation was about to be run but the observer was already disposed: " + operation);
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about?

The operation was about to be run but the observer had been already disposed

@dariuszseweryn dariuszseweryn merged commit eeb4664 into dariuszseweryn:master May 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants