Skip to content
This repository has been archived by the owner on Jul 6, 2018. It is now read-only.

Fail to understand how I should handle connection failures from a client. #184

Closed
akc42 opened this issue Jul 17, 2017 · 3 comments
Closed

Comments

@akc42
Copy link

akc42 commented Jul 17, 2017

I am trying to construct a client that will handle failure conditions correctly. I am currently testing a situation where the client tries to connect to a server that is not running. I can see at the socket level a tls.connect is failing with "Connection Refused", but this doesn't seem to be translating into anything I can use.

Here is a promise I am trying to create for the session

      this.sessionPromise = new Promise((resolve,reject) => {
        function onSessionError(err) {
          session.removeListener('error', onSessionError);
          reject(err);
        }
        const session = http2.connect(
          `https://localhost:${process.env.PAS_HTTPS_PORT}`,
          { rejectUnauthorized: false},
          () => {
            session.removeListener('error', onSessionError);
            resolve(session);
          }
        );
        session.on('error', onSessionError);
      });

but that promise never resolves.

In a previous attempt, I didn't wait for the connection to establish, and after creating the connection immediately created a request - something like this

const session = http2.connect(
          `https://localhost:${process.env.PAS_HTTPS_PORT}`,
          { rejectUnauthorized: false}
);
const req = session.request(headers)
req.on('error', /*did something here */)
req.on('end', /* did something here */)

In that scenario, the 'end' event was raised pretty quickly - but no error.event. Unfortunately that makes it very difficult to understand why the 'end' event was raised, so I didn't pursue it.

What is the correct way of dealing with this? docs don't give much clue.

@jasnell
Copy link
Member

jasnell commented Jul 17, 2017

Looks like this is an area we need to tighten up a bit. I'll look into it.

@akc42
Copy link
Author

akc42 commented Jul 18, 2017

I have solved the problem by listening on socketError on the session, which I got from the code, and then realised that is what the docs said..

@jasnell
Copy link
Member

jasnell commented Jul 19, 2017

Good, ok. I'm going to add a commit to the initial PR that attempts to refine this just a bit. It should not impact the actual flow here so you shouldn't need to change anything.

Essentially the flow is this:

On the client side, when the Socket emits an 'error' event, that is forwarded to the 'socketError' event on the associated ClientHttp2Session. If no 'socketError' event is registered, the 'error' event is re-emitted on the Socket via a call to socket.destroy(error).

On the server side, it's the same except that a default listener for the 'socketError' event on the ServerHttp2Session is registered that forwards the 'socketError' on to the owning Http2Server instance if no other 'socketError' events are registered. This essentially means that the Socket 'error' event will not be re-emitted on the server side.

The big caveat, however, is that application should should never write directly to the Socket associated with an Http2Session or the HTTP/2 session state will get out of sync and will be useless. The only viable option with a 'socketError' event is to tear everything down.

jasnell added a commit that referenced this issue Jul 19, 2017
Fixes: #184

Refines the `'socketError'` event a bit and adds a test for the
emission of the `'socketError'` event on the server. Client side
is tested separately
jasnell added a commit that referenced this issue Jul 22, 2017
Fixes: #184

Refines the `'socketError'` event a bit and adds a test for the
emission of the `'socketError'` event on the server. Client side
is tested separately
jasnell added a commit that referenced this issue Aug 3, 2017
Fixes: #184

Refines the `'socketError'` event a bit and adds a test for the
emission of the `'socketError'` event on the server. Client side
is tested separately
jasnell added a commit to jasnell/node that referenced this issue Aug 13, 2017
Fixes: nodejs/http2#184

Refines the `'socketError'` event a bit and adds a test for the
emission of the `'socketError'` event on the server. Client side
is tested separately

PR-URL: nodejs#14239
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit to nodejs/node that referenced this issue Aug 14, 2017
Fixes: nodejs/http2#184

Refines the `'socketError'` event a bit and adds a test for the
emission of the `'socketError'` event on the server. Client side
is tested separately

Backport-PR-URL: #14813
Backport-Reviewed-By: Anna Henningsen <[email protected]>
Backport-Reviewed-By: Timothy Gu <[email protected]>

PR-URL: #14239
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants