Abort the request if close is called while connecting
#956
Conversation
| this.readyState = WebSocket.CLOSED; | ||
| return; | ||
| this._req.abort(); | ||
| this.emit('error', new Error('closed while connecting')); |
There was a problem hiding this comment.
This is added to match the browser behavior. I'm not sure if we want this error.
I'm also not sure if the error message is ok, suggestions?
| this._req = httpObj.get(requestOptions); | ||
|
|
||
| this._req.on('error', (error) => { | ||
| if (this._req.aborted) return; |
There was a problem hiding this comment.
This is to avoid "socket hang up" errors which are emitted when a request is aborted and a response is not received.
We can remove the check as the error would be swallowed anyway but I think it's better to be explicit here.
|
@websockets/admin please review when/if you can. |
|
Looks sane to me. |
| this.readyState = WebSocket.CLOSED; | ||
| return; | ||
| this._req.abort(); | ||
| this.emit('error', new Error('closed while connecting')); |
There was a problem hiding this comment.
Well it makes sense, so just keep it.
There was a problem hiding this comment.
What about the error message? Can you think of something better?
There was a problem hiding this comment.
Some alternatives:
- "closed before the connection is established"
- "closed before receiving a handshake response"
I have no strong opinions.
There was a problem hiding this comment.
closed before the connection is established is a good one as well.
There was a problem hiding this comment.
Ok, I'll change it to that, merge this and release 2.0.0-beta.0 if there are no objections.
|
@lpinca This breaks engine.io - if you try to connect to a server which is down you get an infinite loop because engine.io calls close when you emit the error in close. |
|
Should this be fixed in Engine.IO? |
|
I guess Engine.IO is not the only one who calls |
|
@3rd-Eden Throwing while connecting deviates from the web API in which What's the proper way to abort a connection with |
|
@julien-f add a listener for the |
|
My bad, I'm mistaken, @lpinca Thanks for your answer and sorry for bothering you :) |
Currently
WebSocket.prototype.close()only sets thereadyStatetoCLOSEDif called before receiving a handshake response. Then the connection is eventually closed when the response is received.This is wrong as the
connectionevent is still emitted on the server.This patch aborts the client request if
WebSocket.prototype.close()is called before receiving a handshake response.Fixes #388