A test to trigger 'earliest > now' assertion #1491
Conversation
|
Is the fix to propagate the |
If I understand this fix correctly, doing this means that the connection will closed when the |
There was a problem hiding this comment.
If I understand this fix correctly, doing this means that the connection will closed when the keep_alive timer is expired.
No, just that we would compute the idle timeout based on whether a keep alive is active. We'd still send a ping when that shorter time elapsed, but we wouldn't have this drift issue you refer to.
One thing that is reassuring about this situation is that it only occurs when the server goes AWOL, as this test demonstrates. That's also troubling also because it's a crash, so it's good to see that you caught this one.
KershawChang
force-pushed
the
h3_upload_test
branch
from
de7f8ff to
d207294
Compare
|
I've moved the test to |
|
Hmm, yeah, this doesn't look right. We should not be closing the connection at this point. I think that we need to look for another approach. |
|
Thinking on this more, I think that we probably need to consider tweaks to the expiry() function and how that works. What we want is to have this track when keep-alive packets were sent and either provide updated estimates for the next timeout based on that or simply switch to reporting the My understanding is that we'll retransmit the keep-alive using the usual logic for lost packets (PTO, etc...) so there is no need to continue to report the keep-alive time once we've initiated the keep-alive. So maybe the idle code needs to be told when the keep-alive is sent so that it can stop reporting the shorter timeout. |
Thanks. I'll think about this. |
|
@KershawChang do we have a way forward for this PR? |
|
Making this a draft PR until it becomes reviewable again. |
KershawChang
force-pushed
the
h3_upload_test
branch
from
91c81a4 to
2b037cc
Compare
@martinthomson , hope I understand this comment correctly. I think we can make Note that I didn't fix test failures in this PR. I'll fix them if you think this is a right approach. Thanks. |
|
I'm seeing some build failures here that are probably worth looking into. |
@martinthomson , I've fixed those test failures. |
|
@KershawChang I merged this into #1875 locally, but the |
Benchmark resultsPerformance differences relative to a71e43d.
Client/server transfer resultsTransfer of 134217728 bytes over loopback.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1491 +/- ##
==========================================
- Coverage 93.20% 93.20% -0.01%
==========================================
Files 111 110 -1
Lines 36025 35763 -262
==========================================
- Hits 33578 33333 -245
+ Misses 2447 2430 -17 ☔ View full report in Codecov by Sentry. |
|
Please take another look at test |
This test demonstrates one possible case that how the assertion is triggered.
Basically, this happens when the state of
IdleTimeoutis set toIdleTimeoutState::AckElicitingPacketSentand we haven't receive any packets after that.Note that in expired function, we set
keep_aliveto false, so the expiry function returns a value larger thannowand we don't think the idle timer is expired.However, when
expiryis called innext_delay, we setkeep_aliveto true, so the value returns fromexpiryis smaller thannow.