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

Check if entry exists in dictionary before delete #355

Merged
merged 1 commit into from Nov 19, 2019

Conversation

NIC619
Copy link
Contributor

@NIC619 NIC619 commented Nov 17, 2019

Ran into KeyError when our peer in a stream accidentally close and reset the stream because handlers for both will try to del streams[stream_id] without checking if the entry still exists.

@NIC619 NIC619 requested a review from mhchia November 17, 2019 13:58
Copy link
Contributor

@mhchia mhchia left a comment

Choose a reason for hiding this comment

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

LGTM. Nice catch.

@@ -96,7 +96,8 @@ def shift(self) -> None:
last_entries: List[CacheEntry] = self.history[len(self.history) - 1]

for entry in last_entries:
del self.msgs[entry.mid]
if entry.mid in self.msgs:
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a minimal improvement but you can also do self.msgs.pop(encry.mid, None). Slightly more robust as it's also threadsafe though admittedly that probably doesn't matter much here.

@NIC619 NIC619 merged commit 74198c7 into libp2p:master Nov 19, 2019
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.

None yet

3 participants