Break queue segment chain if no enumeration is in progress #48296
Conversation
Mono doesn't precisely scan stack which means that, if a QueueSegment ref gets stuck in a stack slot over a long period of time, no future queue segments will be collected and the queue will leak. We break the segment chain when dequeuing the last element from a segment, if no enumerations are in progress. This and the starting of the enumeration are protected by the same lock, which means we cannot miss the start of an enumeration while we are preparing to null the link.
|
Tagging subscribers to this area: @eiriktsarpalis Issue DetailsMono doesn't precisely scan stack which means that, if a QueueSegment ref gets stuck in a stack slot over a long period of time, no future queue segments will be collected and the queue will leak. We break the segment chain when dequeuing the last element from a segment, if no enumerations are in progress. This and the starting of the enumeration are protected by the same lock, which means we cannot miss the start of an enumeration while we are preparing to null the link.
|
|
@stephentoub This is a simple and conservative fix for mono/mono#19665. Even though I didn't reproduce an unbounded memory growth on the testcase from that issue, this change still reduces managed heap size down to a third (with mono). I expect this change to be enough. Without it, ConcurrentQueue (and implicitly the threadpool) are potentially leaky on mono |
| // in case one segment remains pinned in memory (which can happen with mono). We can't race | ||
| // with the start of an enumeration because it needs to first take the cross segement lock. | ||
| if (pendingEnumerations == 0) | ||
| head._nextSegment = null; |
There was a problem hiding this comment.
ugh. This looks racy with other dequeuers
There was a problem hiding this comment.
Yes, this will break dequeues. A dequeuer could have already grabbed the head and needs to be able to fully traverse all linked segments from there.
stephentoub
added
the
NO-MERGE
The PR as it currently stands is broken: |
|
I think we would need some additional synchronization for mono if we want this not to leak. |
|
How would synchronization help to make it less likely to leak with conservative stack scanning? I would expect the usual fix to reduce severity of the leaks with conservative stack scanning is going to be setting the offending references to zero, ideally by the JIT. |
|
@jkotas The potential leak in this case happens if a QueueSegment remains pinned for a long period of time, keeping alive all future segments that will belong to this queue. This PR was an easy attempt to break this queue, with little to no cost, but it races with other lock free dequeuers. I'm just saying that, if we want to safely break this list when the last element is dequeued, we would probably need more synchronisation to detect if another thread is currently dequeuing from the segment we are breaking off. Which I don't know how complicated is to do, but I definitely don't enjoy the idea and we would need to have our own concurrent queue for mono since it would slow down coreclr. You mean setting all stack slots that contain objrefs to null when the var dies, or at method end ? That would be expensive but sounds like a reasonable JIT configuration option for people that run into these types of rare issues. |
ghost
locked as resolved and limited conversation to collaborators
Mono doesn't precisely scan stack which means that, if a QueueSegment ref gets stuck in a stack slot over a long period of time, no future queue segments will be collected and the queue will leak. We break the segment chain when dequeuing the last element from a segment, if no enumerations are in progress. This and the starting of the enumeration are protected by the same lock, which means we cannot miss the start of an enumeration while we are preparing to null the link.