feat: fail test on trapped but unreleased calls #15
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| mClock.Advance(time.Second) | ||
| // the two trapped call instances need to be released on separate goroutines since they each wait for the Now() call | ||
| // to return, which is blocked on both releases happening. If you release them on the same goroutine, in either | ||
| // order, it will deadlock. |
There was a problem hiding this comment.
This is kind of unfortunate. It'd be nice if Release also took a context, considering it can be blocking.
I wonder if there's a case to be made for erroring or warning if multiple traps can capture the same call? Perhaps quartz could also detect deadlocks and have a built-in timeout that fails the test.
There was a problem hiding this comment.
I do like the idea of Release() taking a context, and adding a MustRelease(ctx) which fails the test. Would be API breaking change, tho, unless we only did the latter. Personally I'd rather just break the API to make something safer. It's a real easy upgrade.
In the PR above this in the stack I add some logging, so you get to see that multiple traps capture, and we can make the error messages if your context expires suggest that people check for multiple captures.
| c.releases.Done() | ||
| <-c.complete | ||
| c.apiCall.releases.Done() | ||
| <-c.apiCall.complete |
There was a problem hiding this comment.
Wish we could use t.Context() here but I did some experiments and unless the use-case is very specific, it won't help at all (go test -timeout=5s for instance does not cancel the context before panicing).
Merge activity
|
part of #13
Will fail test with an error message if you trap a call and don't release it.
I've also added 2 test cases to the unit test.
One is a skipped test that you can unskip to see what it looks like if you don't release the trapped call.
One tests what happens if two different traps catch the same call. It gets a little awkward because when you
Release()the call, it waits for the call to complete. One of the main purposes is to ensure that a timer or ticker is set, and if we don't wait for the call to complete, you can't ensure the timer is set before you advance the clock. The awkward consequence is that theRelease()calls will deadlock if they happen on the same goroutine because all traps have to release the call before it will return.We separate out the trapping of the call and releasing of the call so that you have a chance to manipulate the clock before the call returns. But, actually, there are really 3 phases to a trapped call:
After
trap.Wait()returns, we know phase 1 is complete. But,Release()actually conflates phase 2 and 3, so there is no way to release the trap without waiting for phase 3.Generally we don't care that much about the distinction, it's really only in the case of multple traps that you'd need to release without waiting to avoid the deadlock.
We could make those phases explicit:
trap.Wait().Release().WaitForComplete(), but that seems pretty involved for what I think is generally an edge case.WDYT?