Fix race condition in the merge iterator close method #9163
Conversation
ghost
assigned 
jsternberg
force-pushed
the
js-merge-iterator-close-race-condition
branch
4 times, most recently
from
1f279f0
to
bed37b5
Compare
If the close happens when next is being called, it can result in a race condition where the current iterator gets set to nil after the initial check. This also fixes the finalizer so it runs the close method in a goroutine instead of running it by itself. This is because all finalizers run on the same goroutine so a close that takes a long time can cause a backup for all finalizers. This also removes the redundant call to `runtime.SetFinalizer` from the finalizer itself because a finalizer, when called, has already cleared itself.
jsternberg
force-pushed
the
js-merge-iterator-close-race-condition
branch
from
bed37b5
to
a73c3a1
Compare
|
@rbetts this will need to be merged and backported to 1.4. Do you want me to backport this to 1.3 too? |
| runtime.SetFinalizer(itr, nil) | ||
| itr.logger.Error("{{.Name}}Iterator finalized by GC") | ||
| itr.Close() | ||
| go func() { |
There was a problem hiding this comment.
do we have any data that the itr.Close runs for a long time? From my observations
- a finalizer iterator wraps a Merge iterator and maybe the Interrupt iterator
- a Merge iterator wraps 1..N
tsm1.<type>Iteratoriterators - a
tsm1.<type>Iteratorwraps 1..N cursors - closing a cursor decrements any file refs before closing
I would not expect any of those to take any amount of time to clean up.
There was a problem hiding this comment.
The main reason I added this is because the merge iterator now has a lock. The close method needs to wait for next to finish before taking the lock and I didn't want to stall the garbage collector. I know the GC isn't "stop the world" anymore and it appears that finalizers run in a different thread, but the SetFinalizer() documentation still recommends launching a goroutine if it can potentially take awhile.
https://godoc.org/runtime#SetFinalizer
Relevant sections:
SetFinalizer sets the finalizer associated with obj to the provided finalizer function. When the garbage collector finds an unreachable block with an associated finalizer, it clears the association and runs finalizer(obj) in a separate goroutine. This makes obj reachable again, but now without an associated finalizer. Assuming that SetFinalizer is not called again, the next time the garbage collector sees that obj is unreachable, it will free obj.
A single goroutine runs all finalizers for a program, sequentially. If a finalizer must run for a long time, it should do so by starting a new goroutine.
If the possibility that this delays other finalizers is fine, then I can remove the goroutine and that should simplify the test.
There was a problem hiding this comment.
Given there is only one of these created per CreateIterator call, it is fine. I wouldn't want to see 1,000's of these created just to call Close
If the close happens when next is being called, it can result in a race
condition where the current iterator gets set to nil after the initial
check.
This also fixes the finalizer so it runs the close method in a goroutine
instead of running it by itself. This is because all finalizers run on
the same goroutine so a close that takes a long time can cause a backup
for all finalizers. This also removes the redundant call to
runtime.SetFinalizerfrom the finalizer itself because a finalizer,when called, has already cleared itself.