8355225: Test gtest/AsyncLogGtest.java failed at droppingMessage_vm: apparent log corruption

[jdksjolen]

Hi,

We've intermittently seen failures from this test. The testing of the actual semantics of dropping async messages is done throughout the entire test file, and then we have a specific test called droppingMessage. Certainly, this is a bit unexpected, and perhaps not what we want. What I did find, however, is that this test doesn't follow the same pattern as the other ones that do the drop message testing.

The other tests do:

    test_asynclog_drop_messages();
    AsyncLogWriter::flush(); // <- Insert a flush token to the AsyncLogWriter buffer and await for it to be written
    fflush(nullptr); // <- Force all libc writes to be performed

And the droppingMessage test only does test_asynclog_drop_messages(); .

I'd like us to just insert these two lines and see whether this affects what we see in our testing. This doesn't explain the so-called 'log corruption' when we try to read the content. The best explanation (read: guess) I have for that is that we're concurrently reading and writing to the file, and this leads to data races between the reads and writes.

Thanks.

Testing: GHA

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8355225: Test gtest/AsyncLogGtest.java failed at droppingMessage_vm: apparent log corruption (Bug - P3)

Reviewers

• David Holmes (@dholmes-ora - Reviewer)
• SendaoYan (@sendaoYan - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28374/head:pull/28374
$ git checkout pull/28374

Update a local copy of the PR:
$ git checkout pull/28374
$ git pull https://git.openjdk.org/jdk.git pull/28374/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28374

View PR using the GUI difftool:
$ git pr show -t 28374

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28374.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jsjolen! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@jdksjolen This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8355225: Test gtest/AsyncLogGtest.java failed at droppingMessage_vm: apparent log corruption

Reviewed-by: dholmes, syan

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 210 new commits pushed to the master branch:

• 02ff38f: 8363986: Heap region in CDS archive is not at deterministic address
• 902aa4d: 8372120: Add missing sound keyword to MIDI tests
• 152cd4d: 8371956: Convert OopStorage to use Atomic
• ... and 207 more: https://git.openjdk.org/jdk/compare/a0e70c4e9489fc3d8f35c3aec9423fe0839ed0bd...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@jdksjolen The following label will be automatically applied to this pull request:

• hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (c393fd60)

[dholmes-ora]

This does seem to make things more consistent. I'm also not sure it can actually exklain the observed issues though.

Thanks

[sendaoYan]

Hi, I apply the proposed patch and rebuild the test-image, the test still intermittent fails.

[jdksjolen]

Hi, I apply the proposed patch and rebuild the test-image, the test still intermittent fails.

And this is the issue where the logs do show up, but there is no log corruption? It makes sense that that persists, as this code change is meant to ensure that the logs have hit disk before checking for the "missing logs" message. I'm writing to you in the JBS issue for that ticket.

[jdksjolen]

/integrate

[openjdk]

Going to push as commit 72c45a4.
Since your change was applied there have been 228 commits pushed to the master branch:

• 5d3e73b: 8371248: Crash in -Xdoclint with invalid @link
• 473471c: 8369838: Likely invalid assert or function call in jimage.cpp
• a3b1aff: 8372046: compiler/floatingpoint/TestSubNodeFloatDoubleNegation.java fails IR verification
• ... and 225 more: https://git.openjdk.org/jdk/compare/a0e70c4e9489fc3d8f35c3aec9423fe0839ed0bd...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jdksjolen Pushed as commit 72c45a4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.