Race condition in core.js publish method

[edelooff]

This is arguably a small problem that is very unlikely to cause problems, but some thought seems to have been given to prevent race conditions in the given code and something seems to have slipped through.

Working back from the end of the publish method:

• on line 31 it performs return ret !== false;.
• This ret variable is set inside the loop handling subscriber callbacks starting on line 24.
• The array looped over is copied from the subscriber object on line 23 (for what I assume is preventing problems with changes to the subscriber array during event handling).
• On line 19, there is another access to the original subscriber array, to accommodate early exits for zero sub situations.

The problem arises if the last subscriber for an event is removed between execution of lines 19 and 23. The loop will run zero times, ret will not be set and a ReferenceError will be thrown upon execution of line 31.

Chances of this going wrong are slim, because there's very little time between the execution of those two lines, but the solution seems fairly straightforward. Two solutions come to mind, though more optimal ones may exist:

1. Copy the subscriber array early and from then on, only access that. Move line 23 up before 19, line 19 should access the copy
2. Set ret=true before executing the loop (and potentially leave out the early return if "zero loops" doesn't cause a performance problem.

[dcneiner]

@edelooff Have you come across this issue in your usage of Amplfy?

First, I really appreciate the detailed report - it made it very easy to follow your points as you made them.

Having looked at it, I am not sure its something that would actually be able to happen. Given the synchronous nature of the code between lines 19 and 23, as far as I know, in a normal JavaScript run loop, it would be impossible for code outside of this method to remove a subscriber between the execution of those lines. If I am thinking about this incorrectly, please let me know – but I am fairly confident this is not a bug that could happen.

[edelooff]

@dcneiner I've not come across this issue in my usage of Amplify, no.

This mostly came up as I discussed the code with a friend, and from closer inspection is based on a false assumption I made with regards to JavaScript asynchronicity. I had assumed JS was fully asynchronous, meaning that every function can be interrupted at any point to switch context to another (or they run on two threads simultaneously). However, this seems to be not the case:

• Concurrency model and event loop on MDN
• asynchronous tasks execution model

This describes how the whole method (from line 7 to 32) will run without any interruption, contrary to my initial statement in the issue. This means there's indeed no race condition and everything works as intended. False alarm and apologies from my side. Bringing assumptions from other languages doesn't quite work out.

Should it not have been obvious, JavaScript's not really my area of expertise. By this point I'm mostly trying to understand some of the decisions that were made with better understanding than my own.

However, this makes me curious about the array slice that's done on line 23. Is making a copy of the subscriber list necessary? Getting a reference to the subscribers from the full list makes access a little faster and a lot easier on the eyes, but given that there's no concurrent (async) access to the subscriber list, it will not change during execution of the loop. This would mean the copy is a waste of memory, right? That is, the .slice() portion could be left out and functionality remains identical.

[dcneiner]

Ah, ok – that makes sense!

However, this makes me curious about the array slice that's done on line 23. Is making a copy of the subscriber list necessary? Getting a reference to the subscribers from the full list makes access a little faster and a lot easier on the eyes, but given that there's no concurrent (async) access to the subscriber list, it will not change during execution of the loop. This would mean the copy is a waste of memory, right? That is, the .slice() portion could be left out and functionality remains identical.

No, this code is in place because we are using a for loop – and since a callback could request a synchronous unsubscribe in the middle of a synchronous publish cycle, it is important the indexes do not change until our for loop is completed. The relevant test that shows this in action is here: https://github.com/appendto/amplify/blob/master/test/core/unit.js#L221

[edelooff]

Ah yes of course, that was pretty obvious. Thanks for the quick response.

[dcneiner]

Sure thing @edelooff! Thanks for the interest in the code!