RFR: 8309067: gtest/AsyncLogGtest.java fails again in stderrOutput_vm

[Johan Sjölen]

On Mon, 20 Nov 2023 08:27:49 GMT, Xin Liu <xliu at openjdk.org> wrote:

> This patch fixes the flaky test AsyncLogTest. The race condition only occurs in async 
> logging mode in combination of BufferUpdater. BufferUpdater is introduced for testing 
> only. It installs smaller buffers so test code can reliably observe messages dropped.
> AsyncLog drops messages when the current buffer is overflown. 
> 
> In stdout/stderrOutput testcases, we used to mix test_asynclog_ls() and 
> test_asynclog_drop_messages(). The later function uses BufferUpdater. As a result, log
> messages distribute in both old buffer and new buffer. 
> 
> A race condition unfolds in the sequences. Thread names are in brackets.
> 1. [AsyncLogWriter] just calls write().
> 2. [Test] calls the constructor of BufferUpdater. It updates AsyncLogWriter->_buffer_staging.
> 3. [AsyncLogWriter] sees the latest _buffer_staging. It emits nothing. 
> 4. [Test] call the destructor of BufferUpdater. _buffer_staging resumes to 
> the original buffer.
> 5. [AsyncLogWriter] is waken up from _lock.wait. It proceeds to swaps _buffer and _buffer_staging.
> at this point, we lost the log messages in the original buffer. AsyncLogWriter was about to dump them
> at 1.
> 
> Only stdout/stderrOutput testcases interleave the original buffer and small buffer. That's why
> we only encounter flaky failures on them. It's up to the OS scheduling. 
> 
> To eliminate the race condition, we must ensure that all messages write after BufferUpdater.
> test_asynclog_drop_messages() fuses test_asynclog_ls().

Hi,

This looks good to me, happy to see that the issue was only in the testing code and not in production code.

test/hotspot/gtest/logging/test_asynclog.cpp line 248:

> 246: TEST_VM_F(AsyncLogTest, droppingMessage) {
> 247:   if (AsyncLogWriter::instance() == nullptr)
> 248:     return;

Please use braces, even for single statement if-bodies.

-------------

Marked as reviewed by jsjolen (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16728#pullrequestreview-1739962266
PR Review Comment: https://git.openjdk.org/jdk/pull/16728#discussion_r1399290530