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

[BUG] ws/lib/sender.js:162: 'First argument must be a valid error code number' #465

Closed
yw662 opened this issue Jan 1, 2023 · 0 comments · Fixed by #495
Closed

[BUG] ws/lib/sender.js:162: 'First argument must be a valid error code number' #465

yw662 opened this issue Jan 1, 2023 · 0 comments · Fixed by #495
Labels
bug Something isn't working

Comments

@yw662
Copy link

yw662 commented Jan 1, 2023

miniflare occasionally throws like this, see cloudflare/workers-sdk#2476 or cloudflare/workers-sdk#1749,
or here #284 .

The problem comes here

pair.addEventListener("close", (e) => {
if (e.code === 1005 /* No Status Received */) {
ws.close();
} else {
ws.close(e.code, e.reason);
}
});
.

and for ws, it receives:

    if (code === undefined) {
      buf = EMPTY_BUFFER;
    } else if (typeof code !== 'number' || !isValidStatusCode(code)) {
      throw new TypeError('First argument must be a valid error code number');
function isValidStatusCode(code) {
  return (
    (code >= 1000 &&
      code <= 1014 &&
      code !== 1004 &&
      code !== 1005 &&
      code !== 1006) ||
    (code >= 3000 && code <= 4999)
  );
}

Maybe it just needs some tweaks to fit in.

@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Jan 9, 2023
@mrbbot mrbbot added the bug Something isn't working label Jan 9, 2023
@mrbbot mrbbot moved this from Selected for Development to Backlog in workers-sdk Jan 9, 2023
@mrbbot mrbbot moved this from Untriaged to Selected for Development in workers-sdk Jan 9, 2023
mrbbot added a commit that referenced this issue Feb 7, 2023
See #490 for a detailed description of the problem this solves.

Previously, we had `ws`'s automatic `Sec-WebSocket-Protocol` handling
enabled, in addition to adding the header ourselves. This lead to
duplicate sub-protocols in the WebSocket handshake response which is
a protocol error.

Workers require users to include this header themselves in WebSocket
`Response`s, so ignoring this key outright when copying headers would
be incorrect. Instead, we just disable `ws`'s automatic handling.
Funnily enough, we had exactly the same issue in Miniflare 2 too
(#179), and fixed it in 34cc73a. Looks like the fix didn't make it
into Miniflare 3 though. :(

This PR also fixes an issue where `1006` closures were not propagated
correctly. This is an issue in Miniflare 2 too (#465), so we should
back-port this.

Supersedes #490
mrbbot added a commit that referenced this issue Feb 7, 2023
Previously, the abnormal closure close code `1006` was passed
directly to `StandardWebSocket#close()`. This is forbidden, throwing
an uncatchable validation error. With this change, the WebSocket
is `terminate()`ed instead, propagating the `1006` code correctly to
the other side of the connection.

Ref #493
mrbbot added a commit that referenced this issue Feb 9, 2023
See #490 for a detailed description of the problem this solves.

Previously, we had `ws`'s automatic `Sec-WebSocket-Protocol` handling
enabled, in addition to adding the header ourselves. This lead to
duplicate sub-protocols in the WebSocket handshake response which is
a protocol error.

Workers require users to include this header themselves in WebSocket
`Response`s, so ignoring this key outright when copying headers would
be incorrect. Instead, we just disable `ws`'s automatic handling.
Funnily enough, we had exactly the same issue in Miniflare 2 too
(#179), and fixed it in 34cc73a. Looks like the fix didn't make it
into Miniflare 3 though. :(

This PR also fixes an issue where `1006` closures were not propagated
correctly. This is an issue in Miniflare 2 too (#465), so we should
back-port this.

Supersedes #490
mrbbot added a commit that referenced this issue Feb 9, 2023
Previously, the abnormal closure close code `1006` was passed
directly to `StandardWebSocket#close()`. This is forbidden, throwing
an uncatchable validation error. With this change, the WebSocket
is `terminate()`ed instead, propagating the `1006` code correctly to
the other side of the connection.

Ref #493
@github-project-automation github-project-automation bot moved this from Backlog to Done in workers-sdk Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants