Polykey icon indicating copy to clipboard operation
Polykey copied to clipboard

Refactor error handling of failed Node Connections created from the Discovery domain

Open emmacasolin opened this issue 2 years ago • 0 comments

Specification

While completing https://github.com/MatrixAI/js-polykey/pull/311 it was discovered that failed node connections created by the Discovery domain (using NodeManager.requestChainData() to get the sigchain data of a node during the discovery process) were not handled correctly, leading to strange errors during testing. A quick fix for handling these errors was implemented, however it could be refactored to be more specific.

Currently we have (inside src/discovery/Discovery.ts):

// Lines 234-246 (requesting chain data from a node in the discovery queue)
try {
  vertexChainData = await this.nodeManager.requestChainData(
    nodeId,
  );
} catch (e) {
  this.visitedVertices.add(vertex);
  await this.removeKeyFromDiscoveryQueue(vertexId);
  this.logger.error(
    `Failed to discover ${vertexGId.nodeId} - ${e.toString()}`,
  );
  yield;
  continue;
}

// Lines 281-302 (requesting chain data from a node that is linked to a node in the discovery queue)
try {
  linkedVertexChainData =
    await this.nodeManager.requestChainData(linkedVertexNodeId);
} catch (e) {
  if (
    e instanceof nodesErrors.ErrorNodeConnectionDestroyed ||
    e instanceof nodesErrors.ErrorNodeConnectionTimeout
  ) {
    if (!this.visitedVertices.has(linkedVertexGK)) {
      await this.pushKeyToDiscoveryQueue(linkedVertexGK);
    }
    this.logger.error(
      `Failed to discover ${nodesUtils.encodeNodeId(
          linkedVertexNodeId,
       )} - ${e.toString()}`,
    );
    yield;
    continue;
  } else {
    throw e;
  }
}

// Lines 367-387 (requesting chain data from a node that is linked to an identity in the discovery queue)
try {
  linkedVertexChainData = await this.nodeManager.requestChainData(
    linkedVertexNodeId,
  );
} catch (e) {
  if (
    e instanceof nodesErrors.ErrorNodeConnectionDestroyed ||
    e instanceof nodesErrors.ErrorNodeConnectionTimeout
  ) {
    if (!this.visitedVertices.has(linkedVertexGK)) {
      await this.pushKeyToDiscoveryQueue(linkedVertexGK);
    }
    yield;
    this.logger.error(
      `Failed to discover ${data.node} - ${e.toString()}`,
    );
    continue;
  } else {
    throw e;
  }
}

Additional context

  • Original discussion of error propagation in this situation: https://github.com/MatrixAI/js-polykey/pull/311#issuecomment-1049484685
  • Issue for writing tests for the errors that may occur here (progress on these two issues may affect the other): https://github.com/MatrixAI/js-polykey/issues/349

Tasks

  1. Ensure that in each place in the code only the relevant errors are caught and logged out (i.e. ErrorNodeConnectionDestroyed, ErrorNodeConnectionTimeout, and any other errors found while working through #349)
  2. Ensure that after an error is caught the order of subsequent actions is (1) log out the error (2) yield (3) continue

emmacasolin avatar Feb 27 '22 22:02 emmacasolin