javscript: Emit connection error #3733
Conversation
|
Hi @TundraX , thanks a lot for your pull request. For legal reasons, we require contributors to sign a CLA before we can merge a pull request. You can find all the details and sign it online at http://rethinkdb.com/community/cla/. @deontologician can you please take a look at @TundraX's patch? |
|
Yes, taking a look now. @TundraX thanks for the PR, I'll try to review it reasonably quickly, but it may be a day or two |
|
Hi @danielmewes, @deontologician. Just signed the CLA, thank you. I will try to check if the same problem exists with other drivers. |
| @@ -399,7 +399,7 @@ class TcpConnection extends Connection | |||
| @rawSocket.on 'error', (args...) => | |||
| if @isOpen() | |||
| @close({noreplyWait:false}) | |||
| @emit 'close' | |||
| @emit 'error', new err.RqlDriverError "Could not connect to " + @host + ":" + @port | |||
There was a problem hiding this comment.
OK, thanks for being patient, I finally understand the driver code in depth enough to form an educated opinion on this PR.
I agree the "error" event should be raised again instead of "close". But I think we should make three changes to the patch here:
- Change the callback from taking
(args...)to just(err), since Node only passes an error object, not a bunch of args (this was an oversight in the existing code) - Instead of passing a new
RqlDriverErroras the argument toemit, just passerr. In the Connection superclass, it registers a listener for"error"that will raise this same RqlDriverError, so it's redundant here. - Remove the
isOpen()check in here, since (as you mention) the"close"event gets emitted immediately afterwards, it's redundant.
The final code should just be like:
@rawSocket.on('error', (err) => @emit('error', err))
|
Thank you @deontologician. Updated with redundant checks removed. |
javascript: Emit connection error
|
Thanks! |
|
@deontologician is this a breaking change or a bug fix? If it is a bug fix, we should cherry-pick it into the v1.16.x branch and do a release of the JavaScript driver. |
|
Bugfix, see #3807. This brings the driver in line with its expected and documented behavior. |
|
@stuartpb I've released a new version of the JavaScript driver (1.16.1) with this change. |
|
Thanks, I've updated the dependencies of my projects that use connections. |
timeoutconfiguration option has no effect in case of connection refused error (server down). Timeout is cleared in@rawSocket.onceand no error ever emitted. Since@rawSocket.on "close"event is called directly after@rawSocket.on "error"we can just emit a connection error.