Forward socket errors to request callbacks.

[arthurschreiber]

This is based on #325 - thanks to @ashelley for the bug report and initial work.

On top of the original change, it also updates the Connection#transaction helper to work correctly
in case a socket error has happened.

Closes #325.

[lee-houghton]

Hmm, what happens when a socket error is raised when not in the SentClientRequest state (e.g. the session is killed by another connection)? Does it emit an error event on the Connection object?

[arthurschreiber]

Does it emit an error event on the Connection object?

Yes, I think that's what happens. You'll get a error event with a ConnectionError emitted on the connection object. Maybe we should also give the same error object to the request callback? Because right now, we forward the "original" error.

[arthurschreiber]

Check the Connection#socketError method for details.

[lee-houghton]

Yep, I ended up looking at that, and it cleared things up, thanks. I must have previously assumed that the error would be raised on the next request, but this makes more sense.

Is it worth expanding on the definition of the "error" event in the documentation to include examples like this? It's a little Spartan at the moment.

[ashelley]

@arthurschreiber thanks for doing this. Good call on fixing up the transaction path.

Related:

I just spent this morning trying to get tests passing on tedious-connection-pool so that I could potentially fix the second part of the problem noted here:

tediousjs/tedious-connection-pool#23

But I ran out of time for now. The test suite seems to be a bit crufty and requires more debugging time than I have this second. I hope to look at this sooner or later but just wanted to mention it as I still believe this above pull request doesn't have an effect when using the pool because the connection emits the connection error before calling the socket request:

https://github.com/pekim/tedious/blob/arthur/fixup-325/src/connection.js#L485

and because of this... the pool calls connection.close() here:

https://github.com/pekim/tedious-connection-pool/blob/master/lib/connection-pool.js#L85

which causes the driver to transition to STATE.FINAL before trying to dispatch the socketError event so the request callback is still never called.

I think dispatching the socketError before emitting the connection error event will fix it but I didn't want to try that until I got all the current connection pool tests passing. However currently these tests are failing in the pool:

  1) ConnectionPool lost connection handling:
     Error: timeout of 10000ms exceeded. Ensure the done() callback is being called in this test.


  2) ConnectionPool drain:
     Uncaught ConnectionError: Failed to connect to undefined:1433 - connect ECONNREFUSED
      at ConnectionError (/home/adam/src/tedious/lib/errors.js:21:12)
      at Connection.socketError (/home/adam/src/tedious/lib/connection.js:571:58)
      at net.js:459:14

[arthurschreiber]

the connection emits the connection error before calling the socket request:

I checked what some of the "core" node.js modules (e.g. net and socket) do, and they all emit the error event first before passing the error to a more specific callback.

I think the easiest fix would be if the pool would wrap the connection.close call into process.nextTick.

[arthurschreiber]

Actually, there is no need to call connection.close at all if a socket error happens. The pool code should not call this method in case a socket error happens.

[arthurschreiber]

@ashelley I'm gonna merge this as-is. Can you please check again if your issue in the connection pool module gets resolved if you remove the connection.close call in the error handler?

[ashelley]

@arthurschreiber I'm looking into this but having issues with the tedious-connection-pool tests, trying to get to the bottom of it. In the meantime I'd say it sounds sane that if the connection pool simply didn't call connection.close() when err.code === 'ESOCKET' this should fix the issue.

[ashelley]

hello @arthurschreiber I have created a pull request here for the above discussed problem with the connection pool after the above patch gets merged: tediousjs/tedious-connection-pool#24

[ben-page]

@arthurschreiber Hi, I'm the maintainer of tedious-connection-pool.

This doesn't sit right with me. Why does calling connection.close stop the request Request callback from being called with the error? It seems to me that calling connection.close should not have side effects on the rest of Tedious's error handling.

My library aside, I think many users would assume they should always close their connections after an error to avoid leaking the connection. I doesn't seem right that the user should have to check what type of error occurred to know if it's safe to call connection.close.

[arthurschreiber]

@ben-page Thanks for chiming in here. Let me try to explain this a bit.

In all instances where the error event is raised inside the tedious code, connection.close is automatically called, either directly or indirectly via changing the state of the connection to FINAL (which is exactly what connection.close does). There is absolutely no need to explicitly call connection.close when the error event is emitted. This was always the case and is a long standing bug with the connection pool code.

Now, if an error on the underlying socket happens, tedious first emits an error event, then forwards the error to the socketError handler of the current state. As emitting events is a synchronous operation, the connection pool handler gets called in between, changing the state (because of connection.close), and thus causing tedious to loose the reference to the currently executing request.

Does this make things a bit clearer?

[ben-page]

Yes, thank you for your response. There is a huge difference between never needing to call connection.close in response to an error and selectively needing to call it.

[arthurschreiber]

There is a huge difference between never needing to call connection.close in response to an error and selectively needing to call it.

Yep, I agree.