Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

added _id correlation avoiding roles criss cross on race condition #11

Merged
merged 1 commit into from

2 participants

@mbrevoort

This is a fix for issue #10
#10

The role lookup was only considering host and port so if an assume happened on reconnection before the connection end, the free from the original connection would free the reconnection's assume. I added an ID to the role object so that it would only free roles corresponding to the original allocation on that connection.

Incidentally, I can't get the tests to pass on my machine since commit 3226531 though I see that it's testing fine on Travis. I can't spin my wheels anymore on this tonight but this change definitely fixes my issue. The tests fail on resubscribe.js and resubscribe_delay.js with this output.

./node_modules/.bin/tap --tap test/resubscribe.js
# TAP version 13
# resubscribe if connection dies
ok 1 allocate emitted
ok 2 assume emitted
ok 3 free emitted
ok 4 allocate emitted
ok 5 assume emitted
ok 6 free emitted
ok 7 allocate emitted
ok 8 assume emitted
ok 9 free emitted
not ok 10 test count != plan
  ---
    file:   node.js
    line:   192
    column: 40
    stack:  
      - getCaller (/Users/mikebre/development/personal/seaport/node_modules/tap/lib/tap-assert.js:415:17)
      - assert (/Users/mikebre/development/personal/seaport/node_modules/tap/lib/tap-assert.js:20:16)
      - Function.equal (/Users/mikebre/development/personal/seaport/node_modules/tap/lib/tap-assert.js:161:10)
      - Test.end (/Users/mikebre/development/personal/seaport/node_modules/tap/lib/tap-harness.js:116:31)
      - Test._endNice (/Users/mikebre/development/personal/seaport/node_modules/tap/lib/tap-test.js:69:26)
      - Array.0 (native)
      - EventEmitter._tickCallback (node.js:192:40)
    found:  9
    wanted: 6
  ...
ok 11 closed server
# tests 11
# pass  10
# fail  1
not ok 12 test/resubscribe.js
  ---
    timedOut: true
    exit:     1
    command:  "node" "resubscribe.js"
  ...


1..12
# tests 12
# pass  10
# fail  2
@mbrevoort

Just following up; I've been running with this for several days and it definitely did fix the issue.

@substack substack merged commit 243305a into substack:master
@substack
Owner

Thanks for the fixes! Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 8 additions and 3 deletions.
  1. +6 −2 index.js
  2. +2 −1  package.json
View
8 index.js
@@ -1,5 +1,6 @@
var upnode = require('upnode');
var semver = require('semver');
+var identifier = require('identifier');
var EventEmitter = require('events').EventEmitter;
exports.connect = function () {
@@ -224,7 +225,8 @@ exports.createServer = function (opts) {
params.port = port;
params.version = version;
params.role = role;
-
+ params._id = identifier(16);
+
roles[role].push(params);
allocated.push(params);
@@ -267,6 +269,7 @@ exports.createServer = function (opts) {
params.port = port;
params.role = role;
params.version = version;
+ params._id = identifier(16);
roles[role].push(params);
allocated.push(params);
@@ -282,6 +285,7 @@ exports.createServer = function (opts) {
withAddr(function (addr) {
var port = params.port;
var host = params.host || addr;
+ var id = params._id;
if (ports[host]) {
var ix = ports[host].indexOf(port);
@@ -293,7 +297,7 @@ exports.createServer = function (opts) {
Object.keys(roles).forEach(function (role) {
var rs = roles[role];
roles[role] = rs.filter(function (r) {
- var x = r.port === port && r.host === host;
+ var x = r.port === port && r.host === host && r._id === id;
if (x) {
found = {};
Object.keys(r).forEach(function (key) {
View
3  package.json
@@ -29,7 +29,8 @@
"dependencies" : {
"upnode" : "~0.2.2",
"optimist" : "0.3.x",
- "semver" : "1.0.x"
+ "semver" : "1.0.x",
+ "identifier" : "0.0.x"
},
"devDependencies" : {
"tap" : "~0.2.3",
Something went wrong with that request. Please try again.