fix a bug in cancellation delayer #343
Conversation
…it was past grace period, even if parent ctx was not canceled yet
| if err != nil { | ||
| t.Fatalf("1st EnableDelayedCancellationWithGracePeriod failed: %v", err) | ||
| } | ||
| time.Sleep(5 * time.Millisecond) // make sure async ops sorts out |
There was a problem hiding this comment.
You know what I'm going to say about this. Is there a way we can get rid of the Sleep? <-ctx.Done()?
There was a problem hiding this comment.
hmm.. I can't think of a better way to do this here. So the goal of sleep is let it fail if grace period incorrectly starts in EnableDelayedCancellationWithGracePeriod, like before this PR. In this (incorrect) case, after the "async stuff" sorts out, the next call to EnableDelayedCancellationWithGracePeriod should fail. If we don't do this sleep, there's a chance that next line (2nd call to EnableDelayedCancellationWithGracePeriod) would happen the cancellation delayer updates its internal states, which make the 2nd call succeeds. Does this make sense?
There was a problem hiding this comment.
Although it's unlikely this bug will ever happen again. So I think we can also just remove this entirely. What do you think?
There was a problem hiding this comment.
I'm ok with removing this test, because it don't test anything. The sleep is meaningless because there is nothing asynchronous that matters for the test.
But I think if you changed the test to one that canceled the context, and then made sure the second call got a proper error, that would be a worthwhile test.
There was a problem hiding this comment.
I think the async delay comes from sync.Cond and time.Timer. I tried removing the time.Sleep call and the test did not fail for the previous commit (which started grace period in the Enable function). A 1 millisecond sleep managed to fail it though. But I'm worried it's gonna take longer on CI.
There was a problem hiding this comment.
My point is that in this PR, with the time.Timer gone, there is no need for the time.Sleep, it is just wasteful. So I think you're better off with a different test that actually checks the cancellation behavior.
There was a problem hiding this comment.
Ah I see! Sounds good will do!
|
Looks good to me mod comment, thanks! |
| t.Fatalf("1st EnableDelayedCancellationWithGracePeriod failed: %v", err) | ||
| } | ||
| time.Sleep(5 * time.Millisecond) // make sure async ops sorts out | ||
| // parent context is not canceled; second "enable" should succeed even it's |
There was a problem hiding this comment.
Actually I don't really understand this test. The grace period never started because cancel() hasn't been called yet, right? And if it was after the grace period, you would get back a context.Canceled error, right?
There was a problem hiding this comment.
You are right. That's what this PR fixes. Before this PR, grace period started in EnableDelayedCancellationWithGracePeriod. So the second time would fail since it's after the grace period. This PR makes sure the grace period only happens after parent context is canceled. So if it's never canceled, the grace period never starts, so 2nd call to EnableDelayedCancellationWithGracePeriod should still succeed. This added test makes sure after grace period and calling EnableDelayedCancellationWithGracePeriod for the 2nd time, it still succeeds as long as the parent context is not canceled.
There was a problem hiding this comment.
(I tested to make sure the previous delayed_cancellation.go fails this test)
songgao
force-pushed
the
songgao/KBFS-1519
branch
from
f296dad
to
185a4be
Compare
When calling
EnableDelayedCancellationWithGracePeriodthe second time, if it's already past grace period since last timeEnableDelayedCancellationWithGracePeriodwas called, acontext.Canceledwould be returned even if parent context has not been canceled yet.This is an issue for e.g.
Flush()since we enable the context delayer inlibfuse, and the enable function is called again infinalizeMDWriteLocked. For a real server over network, this can cause a lot of cancellations for slow writes.Resolves keybase/client#4254 (hopefully)