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

Add check for max payload size when publishing messages #1211

Merged
merged 2 commits into from
Feb 22, 2024

Conversation

Jarema
Copy link
Member

@Jarema Jarema commented Feb 14, 2024

This adds check if the messages Client is publishing to a server it is connected too does not exceed max_payload.

This unfortunately asks for changing the error from a simple struct that maps error from mpsc error to a new one.
Tests of how disruptive the error change is is under progress, hence the Draft.

We are using new Atomic instead of relying on the server info watcher for performance reasons.

Signed-off-by: Tomasz Pietrek tomasz@nats.io

@Jarema Jarema force-pushed the add-check-for-payload-size branch 3 times, most recently from d62fe39 to 7ef8045 Compare February 14, 2024 19:28
@@ -45,6 +45,14 @@ impl From<tokio::sync::mpsc::error::SendError<Command>> for PublishError {
}
}

pub struct MaxPayloadExceeded(String);
Copy link
Contributor

Choose a reason for hiding this comment

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

This would work a bit better if it:

  • Implemented Debug, Display, Error
  • Had fields for the length of the body and the max payload size, with the string formatting done in the Display implementation

Copy link
Member Author

Choose a reason for hiding this comment

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

I did it just to have an error in Draft.
Sure, your improvement is better, but that still makes this single error inconsistent with other errors, which have ErrorKind.

Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
@Jarema
Copy link
Member Author

Jarema commented Feb 22, 2024

@paolobarbolini Decided to switch to how all the others errors are handled, as it's hardly breaking anyone with current not very useful struct.

@Jarema Jarema marked this pull request as ready for review February 22, 2024 10:32
@Jarema Jarema merged commit ba36b97 into main Feb 22, 2024
11 checks passed
@Jarema Jarema deleted the add-check-for-payload-size branch February 22, 2024 16:39
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