Fix a possible double-borrow during event handling #1512
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
filnet
reviewed
Mar 16, 2020
| if let Some(event) = self.0.events.borrow_mut().pop_front() { | ||
| // Make sure not to let the borrow_mut live during the next handle_event | ||
| let event = { self.0.events.borrow_mut().pop_front() }; | ||
| if let Some(event) = event { |
There was a problem hiding this comment.
I am new to Rust, but this fix looks like it shouldn't be necessary.
Is the borrow_mut alive for the whole scope of the if let?
PS: I am sure the fix is adequate. I am having a hard time understanding the issue and fix Rust wise.
There was a problem hiding this comment.
That's what I assumed also, which is why I ended up writing the bug in the first place 😛
I'd guess it has to do with the scope the value is alive for? Even if a value isn't used, I believe the drop is performed at the end of the scope it lives in. Therefore the old borrow lasted until the end of the if let (or maybe even the outer block) even though it could have been dropped earlier.
There was a problem hiding this comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cargo fmthas been run on this branchcargo docbuilds successfullyCHANGELOG.mdif knowledge of this change could be valuable to usersResolves #1476