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

Attach error codes to all receiver errors #1901

Merged
merged 5 commits into from Jun 15, 2021

Conversation

@pimterry
Copy link
Contributor

@pimterry pimterry commented Jun 14, 2021

Fixes #1892

This change doesn't change any messages or close codes, but does move the existing values into constants linked to the error code, to avoid repeating them at every point where the error is thrown.

@lpinca
Copy link
Member

@lpinca lpinca commented Jun 14, 2021

Can you please only add the codes? I'm not sure I like the "errorDefinition" stuff on lib/constant.js.

@lpinca
Copy link
Member

@lpinca lpinca commented Jun 14, 2021

Also can you prefix them with WS_ERR_ instead of just WS_ and document them in doc/ws.md similar to how it is done here https://nodejs.org/api/errors.html#errors_node_js_error_codes?

@pimterry pimterry force-pushed the receiver-error-codes branch from 22d719f to ff56ba6 Jun 14, 2021
@pimterry
Copy link
Contributor Author

@pimterry pimterry commented Jun 14, 2021

Fair enough, done! Docs updated too.

I've changed it for now, but I do think there's a benefit to commonizing the error details to handle the error cases explicitly and make them more maintainable in future. It means quite a bit of duplication of error params otherwise, where 90% of them are just "[msg], true, 1002, [code]", and it makes it harder to quickly check if the close codes and messages and error codes are used consistently or not for each case. Up to you of course though.

@lpinca
Copy link
Member

@lpinca lpinca commented Jun 14, 2021

Maybe but they are all in one place at the moment so it's ok for now. It also keeps the commit/PR focused at doing only one thing and the added benefit is that there is no need to open a new file to read the error message and close code.

lib/receiver.js Outdated
@@ -285,7 +346,8 @@ class Receiver extends Writable {
RangeError,
'Unsupported WebSocket frame: payload length > 2^53 - 1',
false,
1009
1009,
'WS_ERR_INVALID_DATA_PAYLOAD_LENGTH'
Copy link
Member

@lpinca lpinca Jun 14, 2021

I think "invalid" is a bit misleading here. It's not invalid is not supported by ws.

Copy link
Member

@lpinca lpinca Jun 15, 2021

WS_ERR_UNSUPPORTED_DATA_PAYLOAD_LENGTH?

lib/receiver.js Outdated
'Max payload size exceeded',
false,
1009,
'WS_ERR_INVALID_DATA_PAYLOAD_LENGTH'
Copy link
Member

@lpinca lpinca Jun 14, 2021

How about WS_ERR_MESSAGE_TOO_BIG or something like that? The payload length is not invalid per se. It might be the length of a continuation frame that added to the previous ones makes the message length too big.

Copy link
Contributor Author

@pimterry pimterry Jun 15, 2021

I've renamed the WS_ERR_INVALID_DATA_PAYLOAD_LENGTH code to WS_ERR_UNSUPPORTED_MESSAGE_LENGTH. That's a bit more consistent with the other codes (MESSAGE_TOO_BIG isn't a noun), but it's explicitly a message rather than a single frame, and it's just not supported in this connection rather than completely invalid. Does that work for you?

Copy link
Member

@lpinca lpinca Jun 15, 2021

WS_ERR_UNSUPPORTED_MESSAGE_LENGTH would work for #1901 (comment) where the payload length is greater than 2^53 - 1. However in this case it is not unsupported. The error in this case is that the message length exceeds the maximum allowed message length as defined by the maxPayload option.

Copy link
Contributor Author

@pimterry pimterry Jun 15, 2021

However in this case it is not unsupported. The error in this case is that the message length exceeds the maximum allowed message length as defined by the maxPayload option.

That's what I mean by 'unsupported'. Messages that go beyond it aren't "invalid" - imo invalid means against the specification, i.e. this will never work on any conformant websocket connection.

With 'unsupported' I'm trying to say that it may be valid, and it might work elsewhere, but it won't work on this connection right now (e.g. due to the configured maxPayload option). That matches standard use elsewhere - e.g. close code 1003 ("unsupported data" - valid data in a format that this server can't process) or HTTP 415 ("unsupported media type" - a valid request with a media type that this server refuses to handle).

I think that fits both this case (unsupported due to ws maxPayload config) and the other case (unsupported due to JS limitations).

I see what you mean about data frame size vs total message size though. How about we split the two cases into:

  • WS_ERR_UNSUPPORTED_MESSAGE_LENGTH - overall message is too long (> maxPayload)
  • WS_ERR_UNSUPPORTED_DATA_PAYLOAD_LENGTH - a single data frame is too long (> 2^53-1)

Copy link
Member

@lpinca lpinca Jun 15, 2021

How about we split the two cases into

Works for me and that is what I prefer.

Copy link
Contributor Author

@pimterry pimterry Jun 15, 2021

Great, done 👍

lib/receiver.js Outdated
'Max payload size exceeded',
false,
1009,
'WS_ERR_INVALID_DATA_PAYLOAD_LENGTH'
Copy link
Member

@lpinca lpinca Jun 14, 2021

Ditto.

It's not technically invalid - the standard does not disallow the length
that's used - it's just that this specific endpoint won't accept it.

It's also not necessarily a single data payload - the total length might
come from a series of separate data frames comprising a single message.
@pimterry pimterry force-pushed the receiver-error-codes branch from adc6a13 to c818726 Jun 15, 2021
doc/ws.md Show resolved Hide resolved
doc/ws.md Outdated Show resolved Hide resolved
doc/ws.md Outdated Show resolved Hide resolved
doc/ws.md Outdated Show resolved Hide resolved
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
lpinca
lpinca approved these changes Jun 15, 2021
@lpinca lpinca merged commit c6e3080 into websockets:master Jun 15, 2021
33 checks passed
@lpinca
Copy link
Member

@lpinca lpinca commented Jun 15, 2021

Thank you.

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

Successfully merging this pull request may close these issues.

2 participants