Add test for multiplexer GC rooting. #2415
Conversation
|
|
||
| // Use sync wait and sleep to ensure a more timely GC | ||
| Task.WaitAny(startGC.Task, testTask); | ||
| while (!testTask.IsCompleted) |
There was a problem hiding this comment.
May be worth a timeout exit failure condition here, perhaps after N iterations for some N
Also, to check that it hasn't been collected: really, you need to ask a WeakReference? The test is fine, just: we're not directly testing whether it has been collected - we're testing whether the task completed. These are causally related, sure; but if the test name says it is checking collection: we should probably actually check for collection. Or rename the test.
There was a problem hiding this comment.
+1 tests where failure is hanging forever cause a lot of pain :)
There was a problem hiding this comment.
Note that testTask actually has a timeout via WithTimeout, and without the GC rooting fix this test times out.
Unless you have some good idea, I'll likely just rename the test because a WeakReference could be used to confirm that the multiplexer is not collected before the ping completes, but could not be used to confirm that it was not collected after that as the GC was free to collect it, and this would require a lot of extra code to avoid strong references.
On the other hand because of the strong reference in using in the body of testTask, the task completing implies that the connection was still alive. I'll add a comment about this at the end of testTask.
|
Ohhhh, now I get what Marc meant - this was against the other branch not main. Didn't meant to drop this! @kornelpal I'd like to reopen this targeting main when you can! |
|
Sure, I'll reopen it after pushing some improvements. Note that this needs to be merged after #2408 so that the test is not failing. |
|
Renaming it "something something async task completes something": sure;
(naming is hard)
…On Wed, 29 Mar 2023, 13:58 Kornel Pal, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tests/StackExchange.Redis.Tests/GarbageCollectionTests.cs
<#2415 (comment)>
:
> + await db.PingAsync();
+
+ // Disconnect and don't allow re-connection
+ conn.AllowConnect = false;
+ var server = conn.GetServerSnapshot()[0];
+ server.SimulateConnectionFailure(SimulatedFailureType.All);
+ Assert.False(conn.IsConnected);
+
+ var pingTask = Assert.ThrowsAsync<RedisConnectionException>(() => db.PingAsync());
+ startGC.SetResult(true);
+ await pingTask;
+ }).WithTimeout(5000);
+
+ // Use sync wait and sleep to ensure a more timely GC
+ Task.WaitAny(startGC.Task, testTask);
+ while (!testTask.IsCompleted)
Note that testTask actually has a timeout via WithTimeout, and without the
GC rooting fix this test times out.
Unless you have some good idea, I'll likely just rename the test because a
WeakReference could be used to confirm that the multiplexer is not
collected before the ping completes, but could not be used to confirm that
it was not collected after that as the GC was free to collect it, and this
would require a lot of extra code to avoid strong references.
On the other hand because of the strong reference in using in the body of
testTask, the task completing implies that the connection was still alive.
I'll add a comment about this at the end of testTask.
—
Reply to this email directly, view it on GitHub
<#2415 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAEHMATKY62JPU5FHJIHWDW6QWWPANCNFSM6AAAAAAWLHPI4Y>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
3 participants
Add test for multiplexer GC rooting in #2413.