#2827: Solved issue with thread-unsafety of CommitHashesText #3114

levBagryansky · 2024-04-19T10:51:59Z

PR-Codex overview

The focus of this PR is to refactor the CommitHashesText class by replacing org.cactoos.Text with java.lang.String for simplicity and efficiency.

Detailed summary

Replaced org.cactoos.Text with java.lang.String in CommitHashesText class
Updated constructor to use lambda expression for initializing CACHE
Changed asText method return type to java.lang.String
Added a test method isThreadSafe to check thread safety of CommitHashesText

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

eo-maven-plugin/src/test/java/org/eolang/maven/hash/CommitHashesTextTest.java

levBagryansky · 2024-04-19T12:29:35Z

@volodya-lombrozo please review

volodya-lombrozo · 2024-04-19T13:59:10Z

eo-maven-plugin/src/test/java/org/eolang/maven/hash/CommitHashesTextTest.java

@@ -45,4 +49,23 @@ void downloadsDefaultList() throws Exception {
 Matchers.containsString("master")
 );
 }
+
+ @Test
+ void isThreadSafe() {


@levBagryansky Can we implement it simpler?

@Test void isThreadSafe() { MatcherAssert.assertThat( "Can be used in different threads without NPE", IntStream.range(0, 200) .parallel() .mapToObj(i -> new CommitHashesText()) .map(UncheckedText::new) .map(UncheckedText::asString) .allMatch(Objects::nonNull), Matchers.equalTo(true) ); }

@volodya-lombrozo We need CountDownLatch to make test better. See https://www.yegor256.com/2018/03/27/how-to-test-thread-safety.html

@levBagryansky It's a good point, actually, thank you. However, do we really need to make this test better? If my test fails without Synced it means, it is sufficient enough. I believe we can sacrifice this accuracy in favour of simplicity. What do you think?

@volodya-lombrozo It does not fail without Synced sometimes even now.

@volodya-lombrozo We also want have different 200 threads but .parallel does not give it

@levBagryansky BTW, I tried to run your test and it doesn't fail without Synced, as well, as my solution. How do you reproduce the problem?

@levBagryansky And maybe we don't need all of these 200 threads?

@volodya-lombrozo run again

@volodya-lombrozo I did initially this way , but Pqulice didn't like it.

levBagryansky · 2024-04-19T16:06:57Z

@volodya-lombrozo BTW I think there is a problem with testing of such thing. Note, CACHE is a global variable and if it is calculated in previous test, this test does not have sense,

volodya-lombrozo

@levBagryansky Maybe I miss something. So, please, correct me, if I'm wrong somewhere:

I pulled your changes
I reverted all the changes from CommitHashesText.java (removed Synced)
Run your test isThreadSafe (1000 times)
All the tests pass.

So, I don't understand the intention of this test and this changes since everything works just well without them.

levBagryansky · 2024-04-22T12:03:53Z

@volodya-lombrozo This test must be started first because of

private static final Text CACHE = new Sticky(
            CommitHashesText.asText(CommitHashesText.HOME)
        )

If other test already computed CACHE the test does not have a sense.

levBagryansky · 2024-04-22T12:30:00Z

@yegor256 We have a problem with testing thread-safety of CommitHashesText. Now it has bug because of field

private static final Text CACHE = new Sticky(
            CommitHashesText.asText(CommitHashesText.HOME)
        )

Here CACHE is actually a global variable, but it is not synchronized (Sticky is thread-unsafe).
I solved this problem via Synced class, but we cannot test it. If this other tests uses this class, my test with multiple threads passes since CACHE will already be cached. So we need to reset it somehow.
This is a usual problem of Singleton testing. I see the following options:

Use reflection to reset this private static final Text CACHE variable. But as for me, this is too error-prone, because we initiize variable by our self, so if the CommitHashesText will be changed, the test will be incorrect but still passed.
Copy paste this class and create singleton for testing only. But the problem with consistency still exists.
Don't test it or just add my test. This test will fail if other implementations of CommitHashesText will not be thread-safe after caching of CACHE. Note, currently CACHE is thread-safe if it was already cached so now it passes in ci almost always.

yegor256 · 2024-04-23T03:01:37Z

@levBagryansky maybe the @Isolated annotation from JUnit may help: junit-team/junit5#2142 (what you are looking for is running your particular test in a new isolated virtual machine)

levBagryansky · 2024-04-23T13:31:33Z

@yegor256 according to documentation other tests are not running while this test is executing.

@Isolated is used to declare that the annotated test class should be executed in isolation from other test classes.
When a test class is run in isolation, no other test class is executed concurrently. This can be used to enable parallel test execution for the entire test suite while running some tests in isolation (e. g. if they modify some global resource).

This is necessary for the test, but the problem is that usually previous tests already compute the CACHE.
I also considered @Order, it cannot help us here since it orders tests in test class only.

levBagryansky

@volodya-lombrozo please check

levBagryansky · 2024-04-24T12:10:53Z

eo-maven-plugin/src/main/java/org/eolang/maven/hash/CommitHashesText.java

- private static final Text CACHE = new Sticky(
- CommitHashesText.asText(CommitHashesText.HOME)
- );
+ private static final String CACHE = CommitHashesText.asText(CommitHashesText.HOME);


@volodya-lombrozo please take a look. I think we do not need here Sticky at all. Note, that
CommitHashesText.asText(CommitHashesText.HOME) is calling here anyway, why do we need to wrap it to Sticky. Also, in CommitHashesText.asText(CommitHashesText.HOME) String is already calulated but is wrapping to Text.

yegor256 · 2024-04-24T13:34:29Z

@levBagryansky maybe we should create a new annotation for JUnit: @SeparateVM. It will guarantee that the test is executed in a new VM (new process)

levBagryansky · 2024-04-24T14:05:48Z

@yegor256 according to https://stackoverflow.com/questions/3921322/how-to-fork-jvm, Looks like we would do
System.exec("mvn test -Dtest=\"*..*\"") then.
What do you think about such annotation:
@Reload(CommitHashesText.class) which will use classloader inside somehow

…return

…return, @throws, @exception, @see, @SInCE, @serial, @serialField, @Serialdata, @deprecated, @todo, @checkstyle]'

volodya-lombrozo

@levBagryansky

Context

I have found that the original problem occurred in CommitHashesMap, not in CommitHashesText directly. Here is the log from the last failed pipelines:

2024-04-25T02:12:28.4282253Z Caused by: java.lang.NullPointerException
2024-04-25T02:12:28.4283068Z 	at org.cactoos.text.TextOfScalar.asString(TextOfScalar.java:58)
2024-04-25T02:12:28.4284055Z 	at org.cactoos.text.TextEnvelope.asString(TextEnvelope.java:51)
2024-04-25T02:12:28.4285043Z 	at org.cactoos.text.TextEnvelope.asString(TextEnvelope.java:51)
2024-04-25T02:12:28.4285950Z 	at org.cactoos.text.Split.lambda$new$0(Split.java:127)
2024-04-25T02:12:28.4286757Z 	at org.cactoos.scalar.Checked.value(Checked.java:76)
2024-04-25T02:12:28.4287580Z 	at org.cactoos.scalar.IoChecked.value(IoChecked.java:63)
2024-04-25T02:12:28.4288427Z 	at org.cactoos.scalar.Unchecked.value(Unchecked.java:56)
2024-04-25T02:12:28.4289336Z 	at org.cactoos.iterable.IterableOf.iterator(IterableOf.java:83)
2024-04-25T02:12:28.4290277Z 	at org.cactoos.iterable.Mapped.lambda$new$0(Mapped.java:61)
2024-04-25T02:12:28.4291120Z 	at org.cactoos.scalar.Checked.value(Checked.java:76)
2024-04-25T02:12:28.4292141Z 	at org.cactoos.scalar.IoChecked.value(IoChecked.java:63)
2024-04-25T02:12:28.4293001Z 	at org.cactoos.scalar.Unchecked.value(Unchecked.java:56)
2024-04-25T02:12:28.4293949Z 	at org.cactoos.iterable.IterableOf.iterator(IterableOf.java:83)
2024-04-25T02:12:28.4295104Z 	at org.cactoos.iterable.IterableEnvelope.iterator(IterableEnvelope.java:53)
2024-04-25T02:12:28.4296364Z 	at org.cactoos.iterable.IterableEnvelope.iterator(IterableEnvelope.java:53)
2024-04-25T02:12:28.4297728Z 	at org.cactoos.iterable.Mapped.lambda$new$0(Mapped.java:61)
2024-04-25T02:12:28.4298639Z 	at org.cactoos.scalar.Checked.value(Checked.java:76)
2024-04-25T02:12:28.4299499Z 	at org.cactoos.scalar.IoChecked.value(IoChecked.java:63)
2024-04-25T02:12:28.4300385Z 	at org.cactoos.scalar.Unchecked.value(Unchecked.java:56)
2024-04-25T02:12:28.4301344Z 	at org.cactoos.iterable.IterableOf.iterator(IterableOf.java:83)
2024-04-25T02:12:28.4302498Z 	at org.cactoos.iterable.IterableEnvelope.iterator(IterableEnvelope.java:53)
2024-04-25T02:12:28.4303514Z 	at org.cactoos.map.MapOf.make(MapOf.java:189)
2024-04-25T02:12:28.4304257Z 	at org.cactoos.map.MapOf.<init>(MapOf.java:176)
2024-04-25T02:12:28.4305245Z 	at org.eolang.maven.hash.CommitHashesMap.fromTable(CommitHashesMap.java:93)
2024-04-25T02:12:28.4306638Z 	at org.eolang.maven.hash.CommitHashesMap.<init>(CommitHashesMap.java:69)
2024-04-25T02:12:28.4307813Z 	at org.eolang.maven.hash.CommitHashesMap.<init>(CommitHashesMap.java:53)
2024-04-25T02:12:28.4308998Z 	at org.eolang.maven.SafeMojo.<init>(SafeMojo.java:219)
2024-04-25T02:12:28.4309893Z 	at org.eolang.maven.PrintMojo.<init>(PrintMojo.java:57)
2024-04-25T02:12:28.4310580Z 	... 61 more

Of course, these classes are connected, but mentioning this in the issue or in the PR would help to investigate the problem faster and it would add some sort of context. This information usually helps to create an appropriate unit test. And what is the most important, it will free a reviewer from searching for these logs.

Please add more helpful information and a human-readable description for your PRs or issues. It will significantly speed up the review process.

Sticky

Now, let's return to the problem. Actually, by removing the Sticky usage, you solved the issue. Why? Just run this test:

@Test
void revealsProblemWithSticky() {
    for (int j = 0; j < 10_000; ++j) {
        final Text shared = new Sticky(() -> "oops");
        MatcherAssert.assertThat(
            "Shared text should be thread-safe, but it wasn't",
            IntStream.range(0, 200)
                .parallel()
                .mapToObj(i -> shared)
                .map(UncheckedText::new)
                .map(UncheckedText::asString)
                .allMatch(Objects::nonNull),
            Matchers.equalTo(true)
        );
    }
}

And you will see that it fails, since the Sticky doesn’t guarantee thread-safety.

What about our case?

I would suggest writing something as follows:

@Test
void revealsProblemWithCache() {
    for (int j = 0; j < 10_000; ++j) {
        final Sticky cache = new Sticky(
            () -> "15c85d7f8cffe15b0deba96e90bdac98a76293bb 0.23.17"
        );
        MatcherAssert.assertThat(
            "Should calculate the hash of the commit without an NPE, but it didn't",
            IntStream.range(0, 200)
                .parallel()
                .mapToObj(i -> new CommitHashesMap(new CommitHashesText(cache)::asString))
                .map(chm -> chm.get("0.23.17"))
                .allMatch(Objects::nonNull),
            Matchers.equalTo(true)
        );
    }
}

I would recommend removing the isThreadSafe test. Regarding #3141 , I don’t think we need to go so far; the test above should be sufficient.

levBagryansky · 2024-04-25T15:28:25Z

@volodya-lombrozo

I found the last our class that is not thread-safe: CommitHashesText. It has thread-safety vulnerability. Obviously, other classes that use this class are not thread-safe too: CommitHashMap, SafeMojo and PrintMojo.
So the chain of vulnerability is actually
CommitHashesText => CommitHashMap => SafeMojo => PrintMojo.
So original problem happened directly in CommitHashText
As for Sticky we don't even need this test, because there is written in Sticky documentation, that Sticky doesn’t guarantee thread-safety
As for our case, please think, what should we test. In my opinion we don't need tests here at all. But if we want- We should test behaviour of unit CommitHashText- the smallest our unit, that was thread-unsafe.

volodya-lombrozo · 2024-04-25T16:30:01Z

@levBagryansky

Where did you explain these investigations before I mentioned them? The only thing we have is a single comment, Closes #2827, which leads to an even less descriptive issue. https://www.yegor256.com/2018/04/24/right-way-to-report-bugs.html
I didn’t suggest to add this test to the codebase, this test only for the descriptive purposes to indicate the problem with the Sticky object. Again, if you knew about the problem, why didn’t you explain it in the PR description?
We need the test to prevent these changes from happening again (adding Sticky, for example). If you want and can test the smallest unit, great, it will be even better.

The best solution I see is to remove the static field CACHE; by doing this, we will be able to test CommitHashesText properly without overengineering. Removing the static field seems simpler compared to reinitializing static fields. Moreover it leads to a better design.

levBagryansky · 2024-04-25T17:31:13Z

@volodya-lombrozo

I decided that I already localized the problem when found the unit that was not thread-safe. May be it would be better to paste stacktrace here. But it is still hard to understand how this unit influences here. Should I really copy here snippets with using of Sticky, documentation of Sticky, how it is used in CommitHashesText and how it is used in CommitHashMap and so on? If you want you can easy to find it by yourself. This is really easy. Go to CommitHashesText and see how it is used in sources. It is used in CommitHashMap and ChRemote(BTW this class has the same error) only.
But at the same time the error in CommitHashesText is obvious, I do not think I really should clarify to you why it is not thread safe.
Please, let's close this point. I don't think the argument about this is constructive, so I won't answer it here anymore.
Ok, I did not get why you provided it instead of reference to documentation.
In order to run your test we need to get rid of static field. We need this static field in order to download hashes only once. It seems to me that when a programmer writes multithreaded code, he should be especially careful with static fields. As we can see, it is very difficult to write tests for this. But this is not a reason not to write multithreaded code with static fields at all.

levBagryansky · 2024-05-02T08:18:36Z

@yegor256 Let's just merge this

yegor256 · 2024-05-02T08:38:37Z

@rultor merge

rultor · 2024-05-02T08:40:43Z

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

rultor · 2024-05-02T09:00:01Z

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 18min)

objectionary#2827: Solved issue with thread-unsafety of CommitHashesText

4cda81b

levBagryansky commented Apr 19, 2024

View reviewed changes

eo-maven-plugin/src/test/java/org/eolang/maven/hash/CommitHashesTextTest.java Show resolved Hide resolved

levBagryansky added 8 commits April 19, 2024 14:06

objectionary#2827: Using Synced

3ff8986

objectionary#2827: Unused imports

71ddcb0

objectionary#2827: renamed test to isThreadSafe

9aa854f

objectionary#2827:

bed4bb9

objectionary#2827: service.shutdown();

de72320

objectionary#2827: try-with-finally

3690e97

objectionary#2827: try service.shutdownNow();

955e647

objectionary#2827: give up, use Threads

96eae06

volodya-lombrozo suggested changes Apr 19, 2024

View reviewed changes

volodya-lombrozo reviewed Apr 21, 2024

View reviewed changes

levBagryansky added 4 commits April 24, 2024 14:55

objectionary#2827: Removed redundant lambdas

df19acd

objectionary#2827: Removed redundant lambdas

ac90256

objectionary#2827: Restart ci

9455546

objectionary#2827: Restart ci

70c8f3b

levBagryansky commented Apr 24, 2024

View reviewed changes

levBagryansky mentioned this pull request Apr 24, 2024

#3122: Using of Synced #3123

Merged

levBagryansky added 2 commits April 25, 2024 12:10

objectionary#2827: Added puzzle

c3d18ab

objectionary#2827: Block tags have to appear in the order '[@param, @…

d841ce7

…return, @throws, @exception, @see, @SInCE, @serial, @serialField, @Serialdata, @deprecated, @todo, @checkstyle]'

levBagryansky requested a review from volodya-lombrozo April 25, 2024 11:06

volodya-lombrozo suggested changes Apr 25, 2024

View reviewed changes

volodya-lombrozo mentioned this pull request Apr 25, 2024

@Reload(MyClass.class) annotation #3141

Open

This was referenced Apr 26, 2024

ChRemote has threadsafety vulnerability #3148

Closed

3148 chRemote #3149

Merged

levBagryansky added 2 commits April 29, 2024 13:52

objectionary#2827: Enabled

5214b0d

objectionary#2827: unused import

a3c0e82

rultor merged commit f184d9a into objectionary:master May 2, 2024
20 checks passed

levBagryansky deleted the 2827_concurrency-NPE branch May 2, 2024 10:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2827: Solved issue with thread-unsafety of CommitHashesText #3114

#2827: Solved issue with thread-unsafety of CommitHashesText #3114

levBagryansky commented Apr 19, 2024 •

edited by pr-codex bot

levBagryansky commented Apr 19, 2024

volodya-lombrozo Apr 19, 2024

levBagryansky Apr 19, 2024

volodya-lombrozo Apr 19, 2024

levBagryansky Apr 19, 2024

levBagryansky Apr 19, 2024

volodya-lombrozo Apr 19, 2024

volodya-lombrozo Apr 19, 2024

levBagryansky Apr 19, 2024

levBagryansky Apr 19, 2024

levBagryansky commented Apr 19, 2024 •

edited

volodya-lombrozo left a comment

levBagryansky commented Apr 22, 2024 •

edited

levBagryansky commented Apr 22, 2024 •

edited

yegor256 commented Apr 23, 2024 •

edited

levBagryansky commented Apr 23, 2024

levBagryansky left a comment

levBagryansky Apr 24, 2024

yegor256 commented Apr 24, 2024

levBagryansky commented Apr 24, 2024

volodya-lombrozo left a comment

levBagryansky commented Apr 25, 2024 •

edited

volodya-lombrozo commented Apr 25, 2024

levBagryansky commented Apr 25, 2024 •

edited

levBagryansky commented May 2, 2024

yegor256 commented May 2, 2024

rultor commented May 2, 2024

rultor commented May 2, 2024

#2827: Solved issue with thread-unsafety of CommitHashesText #3114

#2827: Solved issue with thread-unsafety of CommitHashesText #3114

Conversation

levBagryansky commented Apr 19, 2024 • edited by pr-codex bot

PR-Codex overview

Detailed summary

levBagryansky commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levBagryansky commented Apr 19, 2024 • edited

volodya-lombrozo left a comment

Choose a reason for hiding this comment

levBagryansky commented Apr 22, 2024 • edited

levBagryansky commented Apr 22, 2024 • edited

yegor256 commented Apr 23, 2024 • edited

levBagryansky commented Apr 23, 2024

levBagryansky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yegor256 commented Apr 24, 2024

levBagryansky commented Apr 24, 2024

volodya-lombrozo left a comment

Choose a reason for hiding this comment

Context

Sticky

What about our case?

levBagryansky commented Apr 25, 2024 • edited

volodya-lombrozo commented Apr 25, 2024

levBagryansky commented Apr 25, 2024 • edited

levBagryansky commented May 2, 2024

yegor256 commented May 2, 2024

rultor commented May 2, 2024

rultor commented May 2, 2024

levBagryansky commented Apr 19, 2024 •

edited by pr-codex bot

levBagryansky commented Apr 19, 2024 •

edited

levBagryansky commented Apr 22, 2024 •

edited

levBagryansky commented Apr 22, 2024 •

edited

yegor256 commented Apr 23, 2024 •

edited

levBagryansky commented Apr 25, 2024 •

edited

levBagryansky commented Apr 25, 2024 •

edited