Skip to content

Commit

Permalink
Allow timer callbacks to participate in WriteFences.
Browse files Browse the repository at this point in the history
This is a carefully considered change motivated by Tinytest, which
fires timers from a method and then waits for them before returning.
Because the timer callbacks didn't have a WriteFence, we had a race
condition a while back where the test client would quiesce before all
the results came in, which I fixed by giving Tinytest its own timer
functions that kept the environment, including the WriteFence.
I want to tear these special timer functions out now, since they are
necessary (before this commit) for very obscure reasons.  The race
condition is hard to reproduce and is affected by Mongo latency, the
order of the unit tests, etc.  (I reproduced it semi-stably to test
this commit, and it was tricky.)

The change is to give timer callbacks the WriteFence and allow them
to add writes before or after the fence fires.  Writes that they get
in before the fence is armed are included in the fence, and writes
made after the fence fires still succeed (the fence is "retired" and
doesn't complain that it has already fired).  In practice, this means
that methods that care about the writes happening as part of the
method, like Tinytest's run method, can wait for them, and methods
that don't care to wait will just return and let the writes trickle
down the pipe later (as they could before).

In a discussion with Geoff a few weeks ago, he said fences in general
should still complain about late writes unless they are put in a
special mode, so there is now a retire() method.
  • Loading branch information
dgreensp committed Sep 16, 2012
1 parent 6a4f20a commit dcd2641
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 13 deletions.
6 changes: 6 additions & 0 deletions packages/livedata/livedata_server.js
Expand Up @@ -237,6 +237,12 @@ _.extend(Meteor._LivedataSession.prototype, {
// done.
var fence = new Meteor._WriteFence;
fence.onAllCommitted(function () {
// Retire the fence so that future writes are allowed.
// This means that callbacks like timers are free to use
// the fence, and if they fire before it's armed (for
// example, because the method waits for them) their
// writes will be included in the fence.
fence.retire();
self.send({
msg: 'data', methods: [msg.id]});
});
Expand Down
13 changes: 13 additions & 0 deletions packages/livedata/writefence.js
Expand Up @@ -6,6 +6,7 @@ Meteor._WriteFence = function () {

self.armed = false;
self.fired = false;
self.retired = false;
self.outstanding_writes = 0;
self.completion_callbacks = [];
};
Expand All @@ -24,6 +25,9 @@ _.extend(Meteor._WriteFence.prototype, {
beginWrite: function () {
var self = this;

if (self.retired)
return { committed: function () {} };

if (self.fired)
throw new Error("fence has already activated -- too late to add writes");

Expand Down Expand Up @@ -77,5 +81,14 @@ _.extend(Meteor._WriteFence.prototype, {
_.each(self.completion_callbacks, function (f) {f(self);});
self.completion_callbacks = [];
}
},

// Deactivate this fence so that adding more writes has no effect.
// The fence must have already fired.
retire: function () {
var self = this;
if (! self.fired)
throw new Error("Can't retire a fence that hasn't fired.");
self.retired = true;
}
});
15 changes: 2 additions & 13 deletions packages/meteor/timers.js
@@ -1,8 +1,7 @@
_.extend(Meteor, {
// Meteor.setTimeout and Meteor.setInterval callbacks scheduled
// inside a server method are not part of the method invocation, and
// should clear out the CurrentInvocation and CurrentWriteFence
// environment variables.
// inside a server method are not part of the method invocation and
// should clear out the CurrentInvocation environment variable.

setTimeout: function (f, duration) {
if (Meteor._CurrentInvocation) {
Expand All @@ -13,11 +12,6 @@ _.extend(Meteor, {
f = function () { Meteor._CurrentInvocation.withValue(null, f_with_ci); };
}

if (Meteor._CurrentWriteFence) {
var f_with_cwf = f;
f = function () { Meteor._CurrentWriteFence.withValue(null, f_with_cwf); };
}

return setTimeout(Meteor.bindEnvironment(f, function (e) {
// XXX report nicely (or, should we catch it at all?)
Meteor._debug("Exception from setTimeout callback:", e.stack);
Expand All @@ -33,11 +27,6 @@ _.extend(Meteor, {
f = function () { Meteor._CurrentInvocation.withValue(null, f_with_ci); };
}

if (Meteor._CurrentWriteFence) {
var f_with_cwf = f;
f = function () { Meteor._CurrentWriteFence.withValue(null, f_with_cwf); };
}

return setInterval(Meteor.bindEnvironment(f, function (e) {
// XXX report nicely (or, should we catch it at all?)
Meteor._debug("Exception from setInterval callback:", e);
Expand Down

0 comments on commit dcd2641

Please sign in to comment.