-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
RqlDriverError
as 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. |
timeout
configuration option has no effect in case of connection refused error (server down). Timeout is cleared in@rawSocket.once
and no error ever emitted. Since@rawSocket.on "close"
event is called directly after@rawSocket.on "error"
we can just emit a connection error.