Skip to content

Conversation

@MiloszKrajewski
Copy link
Contributor

@MiloszKrajewski MiloszKrajewski commented Aug 29, 2025

ObjectStore.PutAsync was transferring ownership of memory buffer.
Memory buffer was returned to the pool on serialization. Everything was going smooth until performance degradation. In such case retries were needed using same memory buffer which was already returned to the pool so it might have contain some other data.

This PR keeps ownership for the whole upload process and does not transfer it down to serializer.

You could argue that using var memoryOwner may be moved inside while loop, but it will just increase pool churn. There are no other awaits after publishing so this buffer would return to pool for only few CPU cycles.

@mtmk
Copy link
Member

mtmk commented Aug 29, 2025

thanks again @MiloszKrajewski! looks good. just need commit gpg signing. you might have to force push after (since all commits need to be signed),which is fine.

@MiloszKrajewski MiloszKrajewski force-pushed the fix/prevent-object-contamination branch from a910427 to f982658 Compare August 29, 2025 14:10
@MiloszKrajewski MiloszKrajewski force-pushed the fix/prevent-object-contamination branch from f982658 to de25ce5 Compare August 29, 2025 14:12
@MiloszKrajewski
Copy link
Contributor Author

thanks again @MiloszKrajewski! looks good. just need commit gpg signing. you might have to force push after (since all commits need to be signed),which is fine.

Phew, it took me a while. Apparently when your signature uses CRLF git tells you that signature file does not exist. Another deep debugging ;-)

@MiloszKrajewski
Copy link
Contributor Author

Any way to get this released even as preview?
Currently we have very dodgy and convoluted way to include this fix in out binaries.

🙏

@mtmk
Copy link
Member

mtmk commented Sep 1, 2025

Any way to get this released even as preview? Currently we have very dodgy and convoluted way to include this fix in out binaries.

🙏

of course, we should have a new release sometime today 🤞

Copy link
Member

@mtmk mtmk left a comment

Choose a reason for hiding this comment

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

LGTM thanks @MiloszKrajewski great fix also a better design

@mtmk mtmk merged commit e5ac9c9 into nats-io:main Sep 1, 2025
16 of 17 checks passed
mtmk added a commit that referenced this pull request Sep 1, 2025
* don't transfer ownership (#942)
* (from preview) Fix Object Store and JetStream publish retry logic (#937)
* (from preview) Don't suppress SystemExceptions in NatsSubBase.ReceiveAsync (#938)
@mtmk mtmk mentioned this pull request Sep 1, 2025
mtmk added a commit that referenced this pull request Sep 2, 2025
* don't transfer ownership (#942)
* (from preview) Fix Object Store and JetStream publish retry logic (#937)
* (from preview) Don't suppress SystemExceptions in NatsSubBase.ReceiveAsync (#938)
@mtmk
Copy link
Member

mtmk commented Sep 2, 2025

@MiloszKrajewski your fix is just released in https://github.com/nats-io/nats.net/releases/tag/v2.6.8

thank you 👍

fess9999 pushed a commit to TouchPlusIE/nats.net that referenced this pull request Sep 10, 2025
fess9999 pushed a commit to TouchPlusIE/nats.net that referenced this pull request Sep 10, 2025
* don't transfer ownership (nats-io#942)
* (from preview) Fix Object Store and JetStream publish retry logic (nats-io#937)
* (from preview) Don't suppress SystemExceptions in NatsSubBase.ReceiveAsync (nats-io#938)
@mtmk mtmk mentioned this pull request Sep 22, 2025
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.

NATS.Client.ObjectStore.NatsObjException: SHA-256 digest mismatch

2 participants