Skip to content

[nodejs] Fix issue with connection failures silently failing#1480

Closed
dorilla wants to merge 1 commit intoapache:0.9.3from
FiscalNote:feature/send-connection-failure-to-caller
Closed

[nodejs] Fix issue with connection failures silently failing#1480
dorilla wants to merge 1 commit intoapache:0.9.3from
FiscalNote:feature/send-connection-failure-to-caller

Conversation

@dorilla
Copy link

@dorilla dorilla commented Jan 25, 2018

In node, when a connection fails, the caller is not signaled the error, it simply hangs. This solution sends a generic TApplicationException with details on the unreachable service.

a connection failure is either:

  1. Service is unreachable (firewall or the service is just simply not on)
  2. During a service call, the service becomes unavailable

To replicate:

  1. Setup a service with a single endpoint (don't start the service)
  2. Setup a client that calls the service endpoint
  3. Call the service endpoint from the client
  4. An ECONNREFUSED error occurs
    • Notice how the error is only emitted to the event loop, the calling process never gets the signal

Another way to replicate:

  1. Setup a service with a single endpoint
    • code the handler to simulate a long-running piece of work (eg. long-running database query)
    • can be accomplished using a setTimeout(callback, 10000); to simulate the long-running piece of work
  2. Setup a client that calls the service endpoint
  3. Call the service endpoint from the client
  4. After 2 seconds, before the timeout, kill the service process
  5. An ECONNRESET error occurs
    • Notice how the error is only emitted to the event loop, the calling process never gets the signal

With the proposed fix, the callback receives the proper error, as well as the promise option.

@dorilla dorilla changed the title Fix issue with connection failures silently failing [nodejs] Fix issue with connection failures silently failing Jan 25, 2018
Copy link
Contributor

@jeking3 jeking3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be retargeted at the master branch.

@dorilla dorilla changed the base branch from 0.9.3 to master January 30, 2018 17:26
@dorilla dorilla changed the base branch from master to 0.9.3 January 30, 2018 17:26
Copy link
Contributor

@jeking3 jeking3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be retargeted at the master branch and it needs an Apache Jira thrift ticket. See https://thrift.apache.org/docs/HowToContribute

@jeking3
Copy link
Contributor

jeking3 commented Mar 9, 2018

This needs to be retargeted at the master branch and it needs an Apache Jira thrift ticket. See https://thrift.apache.org/docs/HowToContribute

@jeking3
Copy link
Contributor

jeking3 commented Mar 13, 2018

@bananer apart from missing any testing, is this a reasonable enhancement?

@bananer
Copy link
Contributor

bananer commented Mar 15, 2018

@jeking3 This looks reasonable. Should have tests, though.

@dorilla
Copy link
Author

dorilla commented Mar 16, 2018

Apologies, I haven't had the time to update this ticket. I fully intend to, just trying to find the time.

@jeking3
Copy link
Contributor

jeking3 commented Aug 1, 2018

Just a reminder, if you can move this forward based on the conversation above please do. Thanks.

@jeking3 jeking3 changed the base branch from 0.9.3 to master October 25, 2018 00:49
@jeking3 jeking3 changed the base branch from master to 0.9.3 October 25, 2018 00:49
@jeking3
Copy link
Contributor

jeking3 commented Oct 25, 2018

This is targeting the wrong branch. Closing.

@jeking3 jeking3 closed this Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants