Retrieving HTTP response body on failures

[yoshigev]

• [V] I've searched for any related issues and avoided creating a duplicate issue.

Description

This is a feature request for an easy way to retrieve the HTTP response body in case of HTTP failures while establishing the WebSocket.

Currently, the error event that is raised in this case only includes the status code.
By using the unexpected-response event which is intended for such cases, the default behavior of handling the failure is not executed.
My request is to have something similar to unexpected-response with a way to execute the default failure handling.

The motivation for this request is that some services (e.g., Azure Speech-To-Text) reply with 400 response in some cases, having the exact error specified in the body of the response. We are searching for a way to debug those cases.

Reproducible in:

• version: 7.4.4
• Node.js version(s): 12.18.0
• OS version(s): Windows 10

Workaround code

As a workaround (and to show what I'm missing), I created a wrapping class to the WebSocket that performs the action I need (see it working on replit):

const WebSocket = require('ws');
const streamToPromise = require('stream-to-promise');
const { promisify } = require('util');

class WrapWebSocket extends WebSocket {
  constructor(...conf) {
    super(...conf);
    this.on('unexpected-response', this.handleUnexpectedResponse.bind(this));
  }

  // Copied from the internal abortHandshake function of the 'ws' library
  abortHandshake(websocket, stream, message) {
    websocket._readyState = WebSocket.CLOSING;

    const err = new Error(message);
    Error.captureStackTrace(err, this.abortHandshake);

    if (stream.setHeader) {
      stream.abort();
      stream.once('abort', websocket.emitClose.bind(websocket));
      websocket.emit('error', err);
    } else {
      stream.destroy(err);
      stream.once('error', websocket.emit.bind(websocket, 'error'));
      stream.once('close', websocket.emitClose.bind(websocket));
    }
  }

  async handleUnexpectedResponse(req, res) {
    // Gather the body from the response, with a timeout of 2 seconds
    const body = await Promise.race([
      streamToPromise(res),
      (async () => {
        await promisify(setTimeout)(2000);
        return Buffer.from('<timeout waiting for body>');
      })()
    ]);

    console.error(`Websocket got unexpected response ${res.statusCode} with body: '${body.toString()}'`);

    // After gathering the body, perform default behavior
    this.abortHandshake(
      this,
      req,
      `Unexpected server response: ${res.statusCode}`
    );
  }
}

With a small test function:

try {
  const websocket = new WrapWebSocket('http://httpbin.org/json');
  websocket.on('error', (err) => {
    console.error(`Websocket error: ${err.message}`);
  });
} catch (err) {
  console.error(`Caught error: ${err.message}`);
}

Some notes:

1. The abortHandshake function (that is copied from the lib) must only be called after the body is retrieved. Otherwise, the stream will be closed, and the body will not be accessible.
2. The usage of the "stream-to-promise" library is just for the simplicity of the example. Any other method might be used to retrieve the body.
3. I've added a guard for timeout for retrieving the body. If someone intend to use it on real case, a guard should also be added for the length of the body.

Feature request:

I thought of two ways to make this easier to implement:

1. Easy way: expose the abortHandshake function, so the implementation will be at the client responsibility (like done with my wrapper class), but giving him/her a way to execute the default behavior (removing the need to copy the internal function).
2. Complete way: expose an API like a "middleware" for handling the unexpected message, giving it a callback to a "next" callback.

I would love to hear your opinion on this.

Thanks,
Yehoshua

cc @orgads

[lpinca]

What is the point of using the default behavior if you are handling the "unexpected" response yourself? When you are done reading the HTTP response body you can do whatever you want. For example you could emit a custom event that gives you the response status code and body and then eventually call websocket.close() or websocket.terminate() to have the 'error' and 'close' events emitted.

I'm not happy with exposing abortHandshake() because it was not designed to be a public API. This has already been discussed here #377 (comment).

I'm also not happy with a middleware API because I think it is not needed for something as simple as this.

[yoshigev]

Thanks for pointing to the discussion about this issue. I see your point and see that you understood my request, so I'll not try to re-argue the same things.

So I'll go on with another question:
Reading the documentation of unexpected-response and also the test case for it, there is no mention that there is a need to close the socket after handling the event.
Do you think this is mandatory?
What would happen if the socket won't be closed (like in the tests)? Could it lead to a leak?

If it is mandatory, please consider adding a note to the documentation of how the socket should be closed after handling the event.

[lpinca]

What would happen if the socket won't be closed (like in the tests)? Could it lead to a leak?

It depends:

• If you are using a keep alive agent, it would be better to keep the socket open as it could be reused for other requests.
• If you are using no agent (default behavior) the socket must be closed manually.
• If you are using no agent but the server sends the Connection header with the close value it is not necessary to close the socket manually as it would be done automatically.
• If response body is not read the socket must be closed manually.

[yoshigev]

@lpinca, thank you very much for your help and time. This really helped.