Skip to content

Commit

Permalink
Fix race condition where hostID was previously sometimes undefined
Browse files Browse the repository at this point in the history
  • Loading branch information
dumbmatter committed Dec 2, 2017
1 parent 5c8d04e commit 99d8bf0
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 1 deletion.
31 changes: 30 additions & 1 deletion index.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ class PromiseWorker {
_callbacks: Map<number, (string | null, any) => void>;
_errorCallback: ErrorCallback | void;
_hostID: number | void; // Only defined on host
_hostIDQueue: (() => void)[] | void; // Only defined on host
_hosts: Map<number, { port: MessagePort }>; // Only defined on worker
_maxHostID: number;
_queryCallback: QueryCallback;
Expand Down Expand Up @@ -79,6 +80,11 @@ class PromiseWorker {
this._workerType = 'Worker';

self.addEventListener('message', this._onMessage);

// Since this is not a Shared Worker, hostID is always 0 so it's not strictly required to
// send this back, but it makes the API a bit more consistent if there is the same
// initialization handshake in both cases.
this._postMessageBi([MSGTYPE_HOST_ID, -1, 0], 0);
}
} else {
if (worker instanceof Worker) {
Expand Down Expand Up @@ -111,6 +117,7 @@ class PromiseWorker {
}

this._worker = worker;
this._hostIDQueue = [];
}
}

Expand Down Expand Up @@ -157,7 +164,7 @@ class PromiseWorker {

postMessage(userMessage: any, targetHostID: number | void) {
// console.log('postMessage', userMessage, targetHostID);
return new Promise((resolve, reject) => {
const actuallyPostMessage = (resolve, reject) => {
const messageID = messageIDs;
messageIDs += 1;

Expand All @@ -171,6 +178,18 @@ class PromiseWorker {
}
});
this._postMessageBi(messageToSend, targetHostID);
};

return new Promise((resolve, reject) => {
// Outside of worker, don't send a message until hostID is known, otherwise it's a race
// condition and sometimes hostID will be undefined.
if (this._hostIDQueue !== undefined && this._hostID === undefined) {
this._hostIDQueue.push(() => {
actuallyPostMessage(resolve, reject);
});
} else {
actuallyPostMessage(resolve, reject);
}
});
}

Expand Down Expand Up @@ -266,6 +285,16 @@ class PromiseWorker {
const hostID: number | void = message[2];

this._hostID = hostID;

if (this._hostIDQueue !== undefined) {
this._hostIDQueue.forEach((func) => {
// Not entirely sure why setTimeout is needed, might be just for unit tests
setTimeout(() => {
func();
}, 0);
});
this._hostIDQueue = undefined; // Never needed again after initial setup
}
} else if (type === MSGTYPE_HOST_CLOSE) {
if (this._worker !== undefined) {
throw new Error('MSGTYPE_HOST_CLOSE can only be sent to a worker');
Expand Down
18 changes: 18 additions & 0 deletions test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,24 @@ describe('host -> worker', function () {
]);
});

it('makes hostID immediately available', function () {
var worker = new Worker(pathPrefix + 'worker-hostid.js');
var promiseWorker = new PromiseWorker(worker);

return promiseWorker.postMessage('ping').then(function (res) {
assert.equal(res, 0);
}).then(function () {
return new Promise(function (resolve, reject) {
setTimeout(function () {
return promiseWorker.postMessage('ping').then(function (res) {
assert.equal(res, 0);
resolve();
}).catch(reject);
}, 500);
});
});
});

});

describe('worker -> host', function () {
Expand Down
6 changes: 6 additions & 0 deletions test/worker-hostid.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
var PromiseWorker = require('..');
var promiseWorker = new PromiseWorker();
console.log('hi');
promiseWorker.register(function (msg, hostID) {
return hostID;
});

0 comments on commit 99d8bf0

Please sign in to comment.