Skip to content

Conversation

@alxhotel
Copy link
Member

@alxhotel alxhotel commented Aug 9, 2016

Object.keys() creates a new array, so while iterating through those keys, one of the peers could have been evicted from the cache. This makes peek(peerId) return undefined.

@yciabaud
Copy link
Contributor

yciabaud commented Aug 9, 2016

Good catch thank you!

@feross feross merged commit 03fe833 into webtorrent:master Aug 10, 2016
feross added a commit that referenced this pull request Aug 10, 2016
allPeers[peerId].leecher = true

// The peer could be evicted at this point
if (typeof peer !== 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

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

It's usually better to invert if-statements like this one to prevent another level of indentation.

This is better, IMO:

if (peer == null) return

Also, it's better to be defensive and treat null like no peer as well, hence the ==.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in f2786cd

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll keep it in mind for future PRs

@feross
Copy link
Member

feross commented Aug 10, 2016

Thanks @alxhotel -- good catch. I just added you to this repo as a collaborator! 👍

@feross
Copy link
Member

feross commented Aug 10, 2016

8.0.10

@alxhotel
Copy link
Member Author

Thanks :)

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.

3 participants