Use the rc package for refcounting inside the implementation of Client. #515
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes the implementation of Promise.Resolve a bit cleaner.
...called clientCursor. Step 1 in a broader refactoring.
We now use the rc package. There are some test failures that need to be pinned down.
Tests seem to expect this to still be good.
With an initially null Client, the client pointer is nil so this field doesn't even exist.
This fixes a number of tests, but not all of them.
We no longer have a bunch of call sites that need to do other things with the locked value, so let's just acquire it inside the function and simplify the heck out of how this is used.
...as well as naming it as such; there's no other misc. clientHookState data, it's all promise resolution.
...just inline it into compress; there's no other logic in there anyway.
lthibault
previously approved these changes
May 11, 2023
newClientCursor was a bit too hard to read.
lthibault
reviewed
May 12, 2023
lthibault
reviewed
May 12, 2023
lthibault
requested changes
May 12, 2023
...which isn't entirely trivial.
We always pass this around in a mutex anyway, so just move that inside the struct like everything else that has synchronized state.
|
Trying to record outstanding issues:
|
|
Fixed the client leak. |
Fixes a leak. I'm not sure why this worked before the rc refactor.
|
I went ahead and updated the weakrefs test. I'm still a little stumped on TestDuplicateBootstrap, and am getting frustrated. I'm tempted to suggest merging this with the failure in place (maybe marking the test as flaky); the bug seems less serious than the one we're on track to fix, so maybe punt it until later. |
Per some discussion on matrix, the way weakrefs interact with promise resolution has changed, but I don't think we actually care, so this just changes the test to match.
lthibault
reviewed
May 24, 2023
As Louis pointed out, this is a bit weird, and I'm not sure why I changed it.
lthibault
approved these changes
May 25, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This builds on top of #502, #508, and #509. There are still some failing tests, so I'm marking this as a draft, but it's close enough that it's probably worth a look.