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

Keep sub alive when reading channel #506

Merged
merged 3 commits into from
Jun 4, 2024
Merged

Keep sub alive when reading channel #506

merged 3 commits into from
Jun 4, 2024

Conversation

caleblloyd
Copy link
Collaborator

Resolves #499
Resolves #505

Adds GC keepalive calls to ActivityEndingMsgReader in order to prevent sub from being GC'd while the channel reader is being interacted with.

Learned about GCHandle.Alloc from this blog post - Using GC.KeepAlive in async methods

I left RegisterSubAnchor in place for now as I saw it was in-use still in the JetStream project. But if every sub there uses an ActivityEndingMsgReader it should be able to be eliminated.

Signed-off-by: Caleb Lloyd <caleblloyd@gmail.com>
@caleblloyd caleblloyd requested a review from mtmk June 4, 2024 03:30
@mtmk
Copy link
Collaborator

mtmk commented Jun 4, 2024

Very nice! 😎

@mtmk
Copy link
Collaborator

mtmk commented Jun 4, 2024

...just checking the tests. they are still passing for me even after removing the GC magic.

Copy link
Collaborator

@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

This is fixing the issue for SubscribeCore. Tests don't seem to fail (at least for me on my machine) when the GC fix isn't in place but that can be looked into in a follow up PR if you like.

Below are what I had to do to fail the tests when GC fixes weren't in place.

Signed-off-by: Caleb Lloyd <caleblloyd@gmail.com>
@caleblloyd
Copy link
Collaborator Author

Of course... I had it all working then simplified the tests to DRY it up, and I covered up the original failures. Just refactored them back to use Task.Run which now fail again for me when using the original ActivityEndingMsgReader

Signed-off-by: Caleb Lloyd <caleblloyd@gmail.com>
@caleblloyd
Copy link
Collaborator Author

Added a new benchmark for Subscribe, a side effect of the GC Handle is that the WaitAsync... TryRead pattern allocates more than ReadAllAsync on the channel reader now. So we will probably want to change our patterns to use more of ReadAllAsync when accessing the sub reader

Method Mean Error StdDev Gen0 Allocated
SubscribeAsync 1.002 s 0.0076 s 0.0004 s 3000.0000 30.54 MB
CoreWait 1.002 s 0.0016 s 0.0001 s 5000.0000 46.65 MB
CoreRead 1.003 s 0.0112 s 0.0006 s 5000.0000 40.18 MB
CoreReadAll 1.002 s 0.0005 s 0.0000 s 3000.0000 30.62 MB

@caleblloyd caleblloyd merged commit 2ff424c into main Jun 4, 2024
10 checks passed
@caleblloyd caleblloyd deleted the dont-gc-channel branch June 4, 2024 19:47
mtmk added a commit that referenced this pull request Jun 11, 2024
* Add ResumeAtRevision support to KV Watcher (#491)
* prefer sub channel ReadAllAsync and rm SubAnchor (#507)
* Keep sub alive when reading channel (#506)
* Add overload for empty requests (#502)
@mtmk mtmk mentioned this pull request Jun 11, 2024
mtmk added a commit that referenced this pull request Jun 11, 2024
* Add ResumeAtRevision support to KV Watcher (#491)
* prefer sub channel ReadAllAsync and rm SubAnchor (#507)
* Keep sub alive when reading channel (#506)
* Add overload for empty requests (#502)
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.

Subscriptions dropped shortly after registration No reply received
2 participants