On Thu, 22 Apr 2021 14:16:21 GMT, Ralf Schmelter <[email protected]> wrote:
> This fixes a race condition in the CompressionBackend class of the heap dump
> code.
>
> The race happens when the thread iterating the heap wants to write the data
> it has collected. If the compression backend has worker threads, the buffer
> to write would just be added to a queue and the worker threads would then
> compress (if needed) and write the buffer. But if no worker threads are
> present, the thread doing the iteration must do this itself.
>
> The iterating thread checks the _nr_of_threads member under lock protection
> and if it is 0, it assume it would have to do the work itself. It then
> releases the lock and enters the loop of the worker threads for one round.
> But after the lock has been released, a worker thread could be registered and
> handle the buffer itself. Then the iterating thread would wait until another
> buffer is available, which will never happen.
>
> The fix is to take the buffer to write out of the queue in the iterating
> thread under lock protection and the do the unlocking.
Hi Ralf,
your change looks good to me.
Thanks for fixing,
Richard.
src/hotspot/share/services/heapDumperCompression.cpp line 262:
> 260: }
> 261:
> 262: void CompressionBackend::thread_loop() {
You could simplify `CompressionBackend::thread_loop()` further:
void CompressionBackend::thread_loop() {
{
MonitorLocker ml(_lock, Mutex::_no_safepoint_check_flag);
_nr_of_threads++;
}
WriteWork* work = get_work();
while (work != NULL) {
do_compress(work);
finish_work(work);
work = get_work();
}
MonitorLocker ml(_lock, Mutex::_no_safepoint_check_flag);
_nr_of_threads--;
assert(_nr_of_threads >= 0, "Too many threads finished");
ml.notify_all();
}
-------------
Marked as reviewed by rrich (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3628
