Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Send buffer fix #380

Merged
merged 12 commits into from
Feb 8, 2024
Merged

Send buffer fix #380

merged 12 commits into from
Feb 8, 2024

Conversation

mtmk
Copy link
Collaborator

@mtmk mtmk commented Feb 8, 2024

If server drops the connection halfway through sending a large message pipelines reader was not recovering after reconnect. With this fix we consider the pending message sent if there were any socket send errors.

PS I could not reproduce the issue connecting to nats-server (hence used the mock server) but the error can be reproduced if proactive max_payload check is commented out. Just try to publish a payload larger than 1MB then try to publish or ping after that. Even though client reconnects you'd see pipeline exceptions: System.InvalidOperationException: The examined position cannot be less than the previously examined position.

@mtmk mtmk requested a review from caleblloyd February 8, 2024 14:35
@mtmk mtmk marked this pull request as ready for review February 8, 2024 14:35
@caleblloyd
Copy link
Collaborator

PS I could not reproduce the issue connecting to nats-server (hence used the mock server) but the error can be reproduced if proactive max_payload check is commented out.

I think what is happening when the proactive max_payload check is disabled is that nats-server is closing the connection when it gets the Payload Size that exceeds its settings, then the code gets stuck in a reconnect loop where it keeps trying to send that message again.

@mtmk
Copy link
Collaborator Author

mtmk commented Feb 8, 2024

I think what is happening when the proactive max_payload check is disabled is that nats-server is closing the connection when it gets the Payload Size that exceeds its settings, then the code gets stuck in a reconnect loop where it keeps trying to send that message again.

Actually no, pipeline reader fails with The examined position cannot be less than the previously examined position. 🤷‍♂️

mtmk and others added 3 commits February 8, 2024 16:40
Signed-off-by: Caleb Lloyd <caleb@synadia.com>
Signed-off-by: Caleb Lloyd <caleb@synadia.com>
Copy link
Collaborator

@caleblloyd caleblloyd left a comment

Choose a reason for hiding this comment

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

LGTM

@mtmk mtmk merged commit dd1efd3 into main Feb 8, 2024
9 of 10 checks passed
@mtmk mtmk deleted the send-buf-fix branch February 8, 2024 18:04
mtmk added a commit that referenced this pull request Feb 8, 2024
* Send buffer fix (#380)
* Publish - avoid async state machine when possible (#373)
* Reject payloads over the threshold set by server (#378)
* Fix NatsSvcServer start time format so that it's no longer culture aware (#374)
* Expose Headers on NatsSvcMsg (#371)
@mtmk mtmk mentioned this pull request Feb 8, 2024
mtmk added a commit that referenced this pull request Feb 8, 2024
* Send buffer fix (#380)
* Publish - avoid async state machine when possible (#373)
* Reject payloads over the threshold set by server (#378)
* Fix NatsSvcServer start time format so that it's no longer culture aware (#374)
* Expose Headers on NatsSvcMsg (#371)
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants