Emit and Publish Peer Connection Errors

[wooliet]

A zetta instance's pubsub can be used to react to peer client disconnect events. Currently, the peer_client module will emit a "closed" event for both error and close events.

peer_client

self.ws.on('error', function onError(err) {
   //...
  self.emit('closed');
});

The following zetta handle of "error" events (and it publish of a "disconnect" topic) will never be called.

peerClient.on('error', function(error) {
  self.peerRegistry.get(peer.id, function(err, result) {
    result.status = 'failed';
    result.error = error;
    result.connectionId = peerClient.connectionId;
    self.peerRegistry.save(result);

    // peer-event
    self.pubsub.publish('_peer/disconnect', { peer: peerClient });
  });
});

This PR adds emit('error', err) to the peer_client module, and updates the zetta handler to publish a '_peer/error' topic with the error and peerClient objects.

It is not expected to cause issues with current code because the "error" event handler was previously never called.

[AdamMagaluk]

Hi @wooliet,

To start i'd be interested in why and what information you're currently lacking with zetta? Is it just the error message to see why it failed?

I have a concerns about adding another _peer/<event> type of event.

1. When the peer_client disconnects with an error the peer registry will be in an intermittent state. Since in zetta.js we're looking for both closed and error and updating the registry i'm not sure what will always end up in the registry in this case because we're emitting both the events. Code Here
2. If we add the _peer/error event we may need to add it the events that are sent over the /peer-management websocket endpoint. Would it feel strange for a client to receive a disconnect and an error event? Code Here
3. Thinking about the other direction peers connecting to a cloud. Should the cloud zetta also emit a _peer/error event when the peer_socket emits an error? Code Here

I would like to propose an alternative approach. Have the peer_client emit only the error event when the underlying websocket emits error. In zetta.js still have it emit _peer/disconnect but with an additional error property on the object that's emitted. This will keep the peer-management api and our events the same but provide the additional info as well as address the issue that the peer_clients error event is never emitted and thus the registry never says the peer went into a failed state.

[wooliet]

@AdamMagaluk

I have a zetta instance "linked" to another. When the main instance restarts and has a new location, I want to be sure and trigger a reconnect process for the original. When the instance tries to connect to a bad location, the ultimate error is "ECONNREFUSED". I wanted to handle that specific issue.

1. "Emitting both events": Are you sure you're emitting both? I am pretty certain that the peer client error handler is never called (looking here). I scanned a few unit tests but (to be honest) did not spend the time to create one that would confirm this behavior. If the peer registry state is working as expected now, it might be a side-effect of something else, and not because the error handler is executed.
2. "Peer Management": Receiving two distinct events would not feel strange. But it also makes sense to indicate that the disconnect was triggered by an error by including an error with the event data.
3. "Peers to Cloud": Same as above.

"Alternative Approach": Your suggestion is the same as my first pass at this. But when I realized that the "error" event is never actually emitted by peer_client, it seemed cleaner to me to publish a unique topic as opposed adding additional data to existing topics.

[AdamMagaluk]

Regarding 1. Today we are only emitting closed. My proposal would be to have it emit only error here. My concern was that in your PR we are now emitting both here which would lead to both of the handlers in zetta.js to be called at the same time. here

[AdamMagaluk]

@wooliet This is kinda what i'm thinking. 6206dc5

[wooliet]

@AdamMagaluk Yeah, that looks right. I agree it'd be better to not emit both "closed" and "error".

I don't know the implications of adding the error data in event_socket.

[AdamMagaluk]

The small update i made in event_socket would allow ws clients subscribed to /peer-management to see if an error occurred on disconnect. Because we can't send the entire peerClient down the websocket we override the actual data with peerClients.properties() but that doesn't included the error parameter.

[AdamMagaluk]

@wooliet Would you want to include that one small change in event_socket then i'd be happy to get this PR merged in.

[wooliet]

I'm sorry @AdamMagaluk , I missed this.

What specific changes would you like for me to add? Those you linked to in lib/event_socket.js?

[AdamMagaluk]

@wooliet Would be the changes in evet_socket from this branch. 6206dc5

[AdamMagaluk]

+1

[mdobson]

+1

[AdamMagaluk]

Closing but opened new PR with commits and tests. #317