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

Some test and general fixes #588

Merged
merged 4 commits into from
Aug 20, 2024
Merged

Some test and general fixes #588

merged 4 commits into from
Aug 20, 2024

Conversation

matheusd
Copy link
Contributor

Cherry-picked from #586.

That PR has uncovered more races, so I'm trying again with a different approach to eliminate them.

The commits that were unrelated to the actual refactor are in this PR to ease the future efforts.

The arena was reused when it shouldn't after a Release() call.
In the future, it would be ideal if wrongful Release() calls could be
flagged (either through an error or panic).
This moves the check for released messages earlier in the Release() call
for transport messages. This prevents triggering the race detector, due
to the check happening before an attempt is made at accessing the
message (which involves indirection through a segment that may have been
released already).

While the prior code would not have actually caused a fault in current
code (because the conditional checks both for the released flag and
whether the message is nil), checking by the release flag first is
more correct and may prevent future bugs.
Previously this was not enforced by the Encode function. While this
limit is unlikely to be hit in practice, it's important to assert it due
to the type allowing it and a count of segments greater than 2^32
causing an invalid marshaling.
@lthibault lthibault merged commit 396906c into capnproto:main Aug 20, 2024
4 checks passed
@matheusd matheusd deleted the some-fixes branch August 21, 2024 09:50
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.

2 participants