Fix: Initial transition timers (#313)

This commit fixes an issue where transition timers (`faulty` -> `tombstone`) were not scheduled correctly if the membership during a `join`-response contains a `faulty`-member. Instead of duplicating the code that schedules the timers, a method is extracted from the gossip handler and used on the `set`-event (emitted after the initial join).

lib/gossip/state_transitions.js

@@ -52,6 +52,24 @@ StateTransitions.prototype.enable = function enable() {
     });
 };
 
+StateTransitions.prototype.handleUpdate = function handleUpdate(update) {
+    switch (update.status) {
+        case Member.Status.alive:
+        case Member.Status.leave:
+            this.cancel(update);
+            break;
+        case Member.Status.suspect:
+            this.scheduleSuspectToFaulty(update);
+            break;
+        case Member.Status.faulty:
+            this.scheduleFaultyToTombstone(update);
+            break;
+        case Member.Status.tombstone:
+            this.scheduleTombstoneToEvict(update);
+            break;
+    }
+};
+
 StateTransitions.prototype.scheduleSuspectToFaulty = function scheduleSuspectToFaulty(member) {
     var self = this;
     this.schedule(member, Member.Status.suspect, this.suspectTimeout, function suspectToFaulty() {

lib/on_membership_event.js

@@ -62,11 +62,11 @@ function createSetHandler(ringpop) {
 
             ringpop.stat('increment', 'membership-set.' + (update.status || 'unknown'));
 
+            ringpop.stateTransitions.handleUpdate(update);
             if (update.status === Member.Status.alive) {
                 serversToAdd.push(update.address);
             } else if (update.status === Member.Status.suspect) {
                 serversToAdd.push(update.address);
-                ringpop.stateTransitions.scheduleSuspectToFaulty(update);
             }
         }
 
@@ -114,22 +114,7 @@ function createUpdatedHandlerForGossip(ringpop) {
     return function onUpdated(updates) {
         for (var i = 0; i < updates.length; i++) {
             var update = updates[i];
-            switch (update.status) {
-                case Member.Status.alive:
-                case Member.Status.leave:
-                    ringpop.stateTransitions.cancel(update);
-                    break;
-                case Member.Status.suspect:
-                    ringpop.stateTransitions.scheduleSuspectToFaulty(update);
-                    break;
-                case Member.Status.faulty:
-                    ringpop.stateTransitions.scheduleFaultyToTombstone(update);
-                    break;
-                case Member.Status.tombstone:
-                    ringpop.stateTransitions.scheduleTombstoneToEvict(update);
-                    break;
-            }
-
+            ringpop.stateTransitions.handleUpdate(update);
             ringpop.dissemination.recordChange(update);
         }
     };

test/unit/membership_test.js

@@ -280,6 +280,21 @@ testRingpop('set emits an event', function t(deps, assert) {
     membership.set();
 });
 
+testRingpop('set handle updates for state transitions', function t(deps, assert) {
+    assert.plan(1);
+
+    var ringpop = deps.ringpop;
+    ringpop.isReady = false;
+
+    var membership = deps.membership;
+    ringpop.stateTransitions.handleUpdate = function() {
+        assert.pass('state transitions scheduled')
+    };
+
+    membership.makeChange('127.0.0.1:3001', Date.now(), Member.Status.alive);
+    membership.set();
+});
+
 testRingpop('set computes a checksum once', function t(deps, assert) {
     assert.plan(1);
 

test/unit/state_transistion_test.js

@@ -0,0 +1,67 @@
+// Copyright (c) 2016 Uber Technologies, Inc.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+'use strict';
+
+var test = require('tape');
+
+var Member = require('../../lib/membership/member');
+var Ringpop = require('../..');
+
+function createRingpop() {
+    var ringpop = new Ringpop({
+        app: 'test',
+        hostPort: '127.0.0.1:3000'
+    });
+    return ringpop;
+}
+
+handleUpdateSchedulesTimer(Member.Status.alive, false);
+handleUpdateSchedulesTimer(Member.Status.leave, false);
+handleUpdateSchedulesTimer(Member.Status.faulty, true);
+handleUpdateSchedulesTimer(Member.Status.suspect,true);
+handleUpdateSchedulesTimer(Member.Status.tombstone,true);
+
+function handleUpdateSchedulesTimer(state, shouldSchedule) {
+    test('handleUpdate schedules timers for state: ' + state, function t(assert) {
+        var ringpop = createRingpop();
+        var stateTransitions = ringpop.stateTransitions;
+        var address = '127.0.0.1:3001';
+
+        if (!shouldSchedule) {
+            // register fake timer to test if it's cancelled correctly
+            stateTransitions.timers[address] = {state: 'fake'}
+        }
+
+        stateTransitions.handleUpdate({
+            status: state,
+            address: address
+        });
+        if (shouldSchedule) {
+            assert.ok(stateTransitions.timers[address], 'timer scheduled');
+            assert.equal(stateTransitions.timers[address].state, state, 'timer scheduled');
+        } else {
+            assert.notok(stateTransitions.timers[address], 'timer not scheduled');
+        }
+
+        assert.end();
+        ringpop.destroy();
+    });
+}