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 for #712 - don't skip next sub if previous sub unsubscribes itself #715

Merged
merged 2 commits into from Sep 20, 2016

Conversation

Projects
None yet
3 participants
@lukewestby
Member

lukewestby commented Sep 20, 2016

The Problem

It's possible that someone can unsubscribe from a port in JavaScript in the same tick that the port's subscription callback is called. Since the array that stores these subscription callbacks is shared and mutated in place by subscribe and unsubscribe that unsubscribing in the same tick as a callback can cause the following subscription to be skipped. This is all demonstrated and explained in #712.

Prior art

See https://github.com/reactjs/redux/blob/master/src/createStore.js#L63-L67

The fix

This fix solves that issue by making the subs array immutable with respect to unsubscribing and iterating. When a subscription callback drops itself the subs array will be cloned before the callback is spliced out. Then, during iteration when a message comes in a separate reference is made to the subs array so that changes to that array are only picked up once iteration moves on to the next message.

Expected behavior after merging

The result is that when a subscription callback drops itself, we wait for the next message before pulling in changes made to the subs array while processing the previous message. If there are multiple messages waiting for the port, the subscription that was removed will only process the ones that came in before it unsubscribed itself.

Alternatives

I think most alternatives to this would just be variations on the underlying idea of cloning and re-referencing at a particular point. I think this one has the lowest memory and performance cost as opposed to cloning before processing messages, for example.

@process-bot

This comment has been minimized.

Show comment
Hide comment
@process-bot

process-bot Sep 20, 2016

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

process-bot commented Sep 20, 2016

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

@lukewestby lukewestby added the bug label Sep 20, 2016

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Sep 20, 2016

Member

This approach makes sense to me. It seems like the simplest nicest way to do this. Nice work!

Also, this PR is great :D

Member

evancz commented Sep 20, 2016

This approach makes sense to me. It seems like the simplest nicest way to do this. Nice work!

Also, this PR is great :D

@evancz evancz merged commit 23723c0 into master Sep 20, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@lukewestby

This comment has been minimized.

Show comment
Hide comment
@lukewestby

lukewestby Sep 20, 2016

Member

🤘 thanks evan!

Member

lukewestby commented Sep 20, 2016

🤘 thanks evan!

@evancz evancz deleted the luke/port_unsubscribe_drops_sub branch Sep 22, 2016

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