Skip to content

refactor: replace fmt.Errorf with MessageTooBigError #535

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

DanielleMaywood
Copy link

@DanielleMaywood DanielleMaywood commented Aug 15, 2025

Currently, it is not possible to detect a MessageTooBig error without computing err.Error() and seeing if this string contains read limited at. This change introduces a new MessageTooBigError type and implements Error() string to match the previous error string to reduce the risk of breaking any downward consumers that rely on this string matching behavior.

@DanielleMaywood DanielleMaywood marked this pull request as ready for review August 15, 2025 12:07
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring approval to @mafredri

read.go Outdated
}

func (e MessageTooBigError) Error() string {
return fmt.Sprintf("read limited at %v bytes", e.Limit)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to change this to %d, but it was %v before so let's maybe leave it that way 😂

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better safe than sorry 😅

Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering this code-path closes the connection, what use-case are we addressing by detecting the specific error?

read.go Outdated
@@ -17,6 +17,14 @@ import (
"github.com/coder/websocket/internal/util"
)

type MessageTooBigError struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need this, I'd still like to avoid introducing new exported error types. I think a sentinel error would be better here as they're a bit easier to use as well via errors.Is.

var ErrMessageTooBig = errors.New("websocket: message too big")

That also doesn't stop us from adding context (limit) via:

err = fmt.Errorf("%w: %w", ErrMessageTooBig, context)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the change, although I'm concerned about the possibility of breaking downstream consumers if they rely on the current error message

Copy link
Member

@mafredri mafredri Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your concern. However, string matching on errors is never recommended and consumers should expect it to be brittle, especially when it contains a number that may change. That said, we could retain the error format meaning if consumers use "contains" rather than "==" they would be fine:

fmt.Errorf("%w: read limited at %d bytes", ErrMessageTooBig, c.msgReadLimit.Load())

I've been giving this some more thought. And I really dislike introducing YAAS (yet another API surface?).

  • We already have StatusMessageTooBig
  • We already close the connection
  • We call c.writeError which calls c.writeClose and returns an error.

Why don't we just do:

msg := fmt.Sprintf("read limited at %d bytes", c.msgReadLimit.Load())
err := c.writeClose(StatusMessageTooBig, msg)
return err

This will return a CloseError, but that is actually more correct IMO. The status of CloseError can be checked for StatusMessageTooBig. Thoughts?

Calling .Error() on the CloseError will return status = 1009 and reason = read limited at N bytes. This is good enough for a string contains but if we really need to do 100% backwards compatibility. We can create a private error type that stringifies the backwards compatible error, but implements custom methods for error is/as to match on CloseError. I don't think we need to go that far though.

Copy link
Member

@mafredri mafredri Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually looks like I skimmed writeClose, it doesn't return the CloseError, but constructing it would be an option.

But maybe returning CloseError from read would be too unexpected (may seem like peer close), so sentinel approach is perhaps best after all.

read.go Outdated
@@ -520,7 +528,7 @@ func (lr *limitReader) Read(p []byte) (int, error) {
}

if lr.n == 0 {
err := fmt.Errorf("read limited at %v bytes", lr.limit.Load())
err := MessageTooBigError{Limit: lr.limit.Load()}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also update the code-path in ws_js.go?

@DanielleMaywood
Copy link
Author

Considering this code-path closes the connection, what use-case are we addressing by detecting the specific error?

The exact use-case is for better logging here:
https://github.com/coder/coder/blob/205eb29e60cc3b3519b4ff0247d191b0c87e4d75/codersdk/wsjson/decoder.go#L39-L41

It was previously decided to log at a debug log level. Assuming we want to keep that original behavior, but raise it for this specific error, we want to be able to match against it properly (without using string.Contains on the error).

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

Successfully merging this pull request may close these issues.

3 participants