Skip to content
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

The callback listener is not added to the options.server. #2100

Closed
1 task done
issuefiler opened this issue Nov 25, 2022 · 1 comment
Closed
1 task done

The callback listener is not added to the options.server. #2100

issuefiler opened this issue Nov 25, 2022 · 1 comment

Comments

@issuefiler
Copy link

Is there an existing issue for this?

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

Description

The callback argument of the WebSocketServer constructor is not “added as a listener for the "listening" event on the HTTP server when not operating in noServer mode,” if the server option, a pre-created Node.js HTTP/S server, is supplied, which is different than the current documentation.

ws/doc/ws.md

Lines 72 to 95 in afd8c62

### new WebSocketServer(options[, callback])
- `options` {Object}
- `backlog` {Number} The maximum length of the queue of pending connections.
- `clientTracking` {Boolean} Specifies whether or not to track clients.
- `handleProtocols` {Function} A function which can be used to handle the
WebSocket subprotocols. See description below.
- `host` {String} The hostname where to bind the server.
- `maxPayload` {Number} The maximum allowed message size in bytes. Defaults to
100 MiB (104857600 bytes).
- `noServer` {Boolean} Enable no server mode.
- `path` {String} Accept only connections matching this path.
- `perMessageDeflate` {Boolean|Object} Enable/disable permessage-deflate.
- `port` {Number} The port where to bind the server.
- `server` {http.Server|https.Server} A pre-created Node.js HTTP/S server.
- `skipUTF8Validation` {Boolean} Specifies whether or not to skip UTF-8
validation for text and close messages. Defaults to `false`. Set to `true`
only if clients are trusted.
- `verifyClient` {Function} A function which can be used to validate incoming
connections. See description below. (Usage is discouraged: see
[Issue #337](https://github.com/websockets/ws/issues/377#issuecomment-462152231))
- `WebSocket` {Function} Specifies the `WebSocket` class to be used. It must
be extended from the original `WebSocket`. Defaults to `WebSocket`.
- `callback` {Function}

ws/doc/ws.md

Lines 173 to 174 in afd8c62

`callback` will be added as a listener for the `'listening'` event on the HTTP
server when not operating in "noServer" mode.


This is because the callback argument is only used if (options.port != null).

ws/lib/websocket-server.js

Lines 87 to 117 in afd8c62

if (options.port != null) {
this._server = http.createServer((req, res) => {
const body = http.STATUS_CODES[426];
res.writeHead(426, {
'Content-Length': body.length,
'Content-Type': 'text/plain'
});
res.end(body);
});
this._server.listen(
options.port,
options.host,
options.backlog,
callback
);
} else if (options.server) {
this._server = options.server;
}
if (this._server) {
const emitConnection = this.emit.bind(this, 'connection');
this._removeListeners = addListeners(this._server, {
listening: this.emit.bind(this, 'listening'),
error: this.emit.bind(this, 'error'),
upgrade: (req, socket, head) => {
this.handleUpgrade(req, socket, head, emitConnection);
}
});
}


I’m not sure if this is a bug or intended behavior. Either the code or the documentation needs to be corrected.

ws version

8.11.0

Node.js Version

19.1.0

System

No response

Expected result

The WebSocketServer constructor’s callback argument being added as a listener for the "listening" event on the given HTTP(S) server if (options.port === null && options.server).

Actual result

The expected not happening.

Attachments

No response

@issuefiler issuefiler changed the title The callback listener is not added to the server. The callback listener is not added to the options.server. Nov 25, 2022
@lpinca
Copy link
Member

lpinca commented Nov 25, 2022

I’m not sure if this is a bug or intended behavior.

It is intended behavior because a user can provide an already listening server.

@lpinca lpinca closed this as completed in ea76193 Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants