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

Removes an unnecessary lock acquisition in order to prevent a deadlock #119

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@bgrozev
Member

bgrozev commented Jan 4, 2016

reported by Swapnil Bagadia (and reduce the risk of deadlocks in
general).

Removes an unnecessary lock acquisition in order to prevent a deadlock
reported by Swapnil Bagadia (and reduce the risk of deadlocks in
general).
@lyubomir

This comment has been minimized.

Show comment
Hide comment
@lyubomir

lyubomir Jan 4, 2016

Member

Well... I agree that the original code before the modification wasn't good at protecting the TransportManager.channels List and, consequently, removing the synchronized block shouldn't make a (theoretically) bad situation worse. I'd rather we at least fix the (theoretically) possible ConcurrentModificationException (which I've fixed locally and I'm currently testing) in addition to fixing the deadlock. Anyway, @bgrozev, please feel free to merge your pull request.

Member

lyubomir commented Jan 4, 2016

Well... I agree that the original code before the modification wasn't good at protecting the TransportManager.channels List and, consequently, removing the synchronized block shouldn't make a (theoretically) bad situation worse. I'd rather we at least fix the (theoretically) possible ConcurrentModificationException (which I've fixed locally and I'm currently testing) in addition to fixing the deadlock. Anyway, @bgrozev, please feel free to merge your pull request.

@gpolitis

This comment has been minimized.

Show comment
Hide comment
@gpolitis

gpolitis Jan 4, 2016

Member

This looks fairly innocent to me as well, especially given that protecting the list is broken anyway.

Member

gpolitis commented Jan 4, 2016

This looks fairly innocent to me as well, especially given that protecting the list is broken anyway.

@bgrozev bgrozev closed this Jan 6, 2016

@bgrozev bgrozev deleted the remove-synchronized-block branch Jan 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment