-
Notifications
You must be signed in to change notification settings - Fork 14
Add sequence ID to emit calls to prevent race #32
Conversation
@@ -41,7 +42,14 @@ module.exports = exports = function(options) { | |||
emit: function(after) { | |||
function checkComplete() { | |||
if (isComplete()) { | |||
setImmediate(emit); | |||
var associatedRequest = requestId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that the emit event meant that the page was complete and ready to be transferred to the client. Am I misunderstanding something there? If not, why are we performing any action once the initial emit has been triggered? And, what happens in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Emit has two forms of deferred operations, ajax and events, in addition to the "immediate" mode. Under these modes the actual emit might occur far later than the original call due to pending events. Calling emit multiple times generally shouldn't have any effect other than moving the emitter to the most restrictive mode, i.e. events -> ajax -> "immediate". It's also possible for multiple emit()
calls to occur prior to actually outputting data as the actual emit logic is run in a setImmediate
call and the event continues executing as well as any queued immediate events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: associatedRequestId
outside of my probably stupid question, +1 |
setImmediate(function() { | ||
// Avoid a race condtion where pooled requests may come in while we still have | ||
// pending emit calls. This will generally only happen for very chatty emit callers. | ||
if (requestId === associatedRequest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So calling page.navigate()
will NOP a pending emit request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. Navigate should stop everything dead.
If a page calls emit multiple times it's possible that the pool output may be preemptively emitted on subsequent requests if they occur within a short enough period. Adding a unique sequence identified to NOP any subsequent operations.
Add sequence ID to emit calls to prevent race
Released in 0.3.0 |
If a page calls emit multiple times it's possible that the pool output may be
preemptively emitted on subsequent requests if they occur within a short
enough period. Adding a unique sequence identified to NOP any subsequent
operations.