Atmosphere / atmosphere-javascript

atmosphere-javascript
Apache License 2.0
122 stars 98 forks source link

connection leak for websocket #255

Closed mihaisdm closed 4 years ago

mihaisdm commented 4 years ago

In case unsubscribe called before connection is fully established To reproduce:

function test() {
  atmosphere.subscribe(request);
  atmosphere.unsubscribe();
}

To fix this issue

_websocket.onopen = function (message) {
                    if (_websocket == null) { //bug-fix
                        this.close();
                        _close();
                        return;
                    }

                    _debug("websocket.onopen");

 _websocket.onmessage = function (message) {
                    if (_websocket == null) { //bug-fix
                        this.close();
                        _close();
                        return;
                    }

                    _debug("websocket.onmessage");
jfarcand commented 4 years ago

@mihaisdm Can you do a pull request so you can get the credit for the fix? As soon as you do it I will release a new official version. Thanks!

SuperPat45 commented 4 years ago

I notice, after this commit on the websocket open function:

                _websocket.onopen = function (message) {
                    if (_websocket == null) {
                        this.close();
                        _close();
                        return;
                    }

                    _debug("websocket.onopen");
                    if (_request.connectTimeout <= 0)
                        _timeout(_request);
                    offline = false;

                    if (_canLog('debug')) {
                        atmosphere.util.debug("Websocket successfully opened");
                    }

                    var reopening = webSocketOpened;

                    if (_websocket != null) {
                        _websocket.canSendMessage = true;
                    }

                    if (!_request.enableProtocol) {
                        webSocketOpened = true;
                        if (reopening) {
                            _open('re-opening', "websocket", _request);
                        } else {
                            _open('opening', "websocket", _request);
                        }
                    }

                    if (_websocket != null) {
                        if (_request.method === 'POST') {
                            _response.state = "messageReceived";
                            _websocket.send(_request.data);
                        }
                    }
                };

The two if (_websocket != null) become useless.

SuperPat45 commented 4 years ago

More other, are you sure that calling the close() method in onopen or onmessage doesn't that risk shutting down a potential opened fallback long-pooling connection if these methods called after connect timeout defined in request.connectTimeout  ?

It might be wiser to check that a fallback connection hasn't opened up in the meantime.

jfarcand commented 4 years ago

@SuperPat45 You are right...I've missed that part. Pull request welcomed if you have the time. Thanksz!