Unload taskListManager by instance, not taskListID #5101
Conversation
Shaddoll
force-pushed
the
refactor-matching
branch
from
1175097
to
713cabc
Compare
Pull Request Test Coverage Report for Build 018656ac-ad37-41bc-9875-ff7d5930f3f7
💛 - Coveralls |
|
|
||
| delete(e.taskLists, *id) | ||
| currentTlMgr, ok := e.taskLists[*id] | ||
| if ok && tlMgr == currentTlMgr { |
There was a problem hiding this comment.
I would add a comment as to why your'e checking both ID and instance referencing the description at temporalio/temporal#1917, since honestly it's a little confusing without looking at the PR and surrounding comment
| if ok && tlMgr == currentTlMgr { | ||
| delete(e.taskLists, *id) | ||
| } | ||
| e.taskListsLock.Unlock() |
There was a problem hiding this comment.
nit: Would there be any reason to not defer this above the lock?
it's currently fine I think now, but if there's any other control flow changes It'd be easy to miss
| } | ||
| delete(e.taskLists, *id) | ||
| e.taskListsLock.Unlock() |
There was a problem hiding this comment.
maybe we can defer this too and only have a single instance of unlock?
There was a problem hiding this comment.
I think we want/need it to unlock before Stop() runs :\
in a more general sense though, yeah - I've been hoping we can come up with a better structure for these kinds of things. Now that we have generics, it might even be palatable to use an IIFE...
What changed?
cherry pick temporalio/temporal#1917
Why?
see temporalio/temporal#1917
How did you test it?
Potential risks
Release notes
Documentation Changes