Skip to content

ws.terminate() fires the 'close' event 30 secs after termination? #891

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

Closed
steffenagger opened this issue Nov 13, 2016 · 4 comments · Fixed by #1033
Closed

ws.terminate() fires the 'close' event 30 secs after termination? #891

steffenagger opened this issue Nov 13, 2016 · 4 comments · Fixed by #1033
Labels

Comments

@steffenagger
Copy link

When ws.terminate() (WebSocket.prototype.terminate) is called, the connection is immediately ended, yet cleanupWebsocketResources is called with a timeout of 30 seconds (a time hardcoded for closeTimeout)?

cleanupWebsocketResources emits the 'close' event. So terminating a connection, with eventHandlers registered for the 'close' event, will then trigger these 30 sec after termination.

Is this intentional? If so, why?

If there is something I have misinterpreted/misunderstood, please enlighten me (is terminate only supposed to be called internally? In that case, why expose it on WebSocket ?) - to me it seems weird that:
A connection that is forcefully terminated waits to clean up, and more importantly, waits to fire the 'close' event as part of the awaited cleanup.


For convenience, here is the code I am referring to, at the time of writing:

Hardcoded timeout :

var closeTimeout = 30 * 1000; // Allow 30 seconds to terminate the connection cleanly

WebSocket.prototype.terminate :

/**
 * Immediately shuts down the connection
 *
 * @api public
 */
WebSocket.prototype.terminate = function terminate () {
  if (this.readyState === WebSocket.CLOSED) return;

  if (this._socket) {
    this.readyState = WebSocket.CLOSING;

    // End the connection
    try {
      this._socket.end();
    } catch (e) {
      // Socket error during end() call, so just destroy it right now
      cleanupWebsocketResources.call(this, true);
      return;
    }

    // Add a timeout to ensure that the connection is completely
    // cleaned up within 30 seconds, even if the clean close procedure
    // fails for whatever reason
    // First cleanup any pre-existing timeout from an earlier "terminate" call,
    // if one exists.  Otherwise terminate calls in quick succession will leak timeouts
    // and hold the program open for `closeTimout` time.
    if (this._closeTimer) { clearTimeout(this._closeTimer); }
    this._closeTimer = setTimeout(cleanupWebsocketResources.bind(this, true), closeTimeout);
  } else if (this.readyState === WebSocket.CONNECTING) {
    cleanupWebsocketResources.call(this, true);
  }
};

cleanupWebsocketResources (this.emit('close', ...) is called):

function cleanupWebsocketResources (error) {
  if (this.readyState === WebSocket.CLOSED) return;

  this.readyState = WebSocket.CLOSED;

  clearTimeout(this._closeTimer);
  this._closeTimer = null;

  // If the connection was closed abnormally (with an error), or if
  // the close control frame was not received then the close code
  // must default to 1006.
  if (error || !this._closeReceived) {
    this._closeCode = 1006;
  }
  this.emit('close', this._closeCode || 1000, this._closeMessage || '');

  if (this._socket) {
    if (this._ultron) this._ultron.destroy();
    this._socket.on('error', function onerror () {
      try {
        this.destroy();
      } catch (e) {}
    });

    try {
      if (!error) this._socket.end();
      else this._socket.destroy();
    } catch (e) { /* Ignore termination errors */ }

    this._socket = null;
    this._ultron = null;
  }

  if (this._sender) {
    this._sender = this._sender.onerror = null;
  }

  if (this._receiver) {
    this._receiver.cleanup();
    this._receiver = null;
  }

  if (this.extensions[PerMessageDeflate.extensionName]) {
    this.extensions[PerMessageDeflate.extensionName].cleanup();
  }

  this.extensions = null;

  this.removeAllListeners();
  this.on('error', function onerror () {}); // catch all errors after this
  delete this._queue;
}
@lpinca
Copy link
Member

lpinca commented Nov 14, 2016

In the worst case scenario the close event is emitted after 30 sec, yes, but in normal circumstances the end event is emitted on the other peer.

Upon receiving this event the other peer closes the connection and this make the same end event to be fired on the peer that called terminate.

cleanupWebsocketResources is called on the original peer when this event is received and the close event is emitted.

I agree that this seems cumbersome and unnecessary, especially because terminate is supposed to close the connection immediately. I don't know why it was designed like this.

@steffenagger
Copy link
Author

steffenagger commented Nov 16, 2016

Ok, thank you for your answer @lpinca !

I'm working in an environment where clients rarely gracefully disconnects (they power off), so I'm in the worst case scenario most of the time :)

I've moved to a different library. So feel free to close/discard this issue, if the current implementation is as intended.

@lpinca
Copy link
Member

lpinca commented Nov 16, 2016

I guess you were using WebSocket.prototype.terminate() to shut down zombie connections as your clients do not send a FIN packet or a close frame right?

If so I totally understand your use case.

@steffenagger
Copy link
Author

Yes exactly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants