Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix rare ConcurrentModificationException when running with Parallel-Ant executor. #81

Closed

Conversation

@mharmer
Copy link
Contributor

commented Dec 11, 2018

This resolves a rare race condition when running with the Parallel-Ant executor.

This seems to be triggered concurrently when a reference was being added to the project at the same time the references were being copied through Project.getCopyOfReferences(). The stack trace observed in this case was:

java.util.ConcurrentModificationException
        at java.util.Hashtable$Enumerator.next(Hashtable.java:1387)
        at java.util.HashMap.putMapEntries(HashMap.java:512)
        at java.util.HashMap.<init>(HashMap.java:490)
        at org.apache.tools.ant.Project.getCopyOfReferences(Project.java:2038)
        at org.apache.tools.ant.util.ScriptRunnerBase.bindToComponent(ScriptRunnerBase.java:307)
        at org.apache.tools.ant.util.ScriptRunnerHelper.getScriptRunner(ScriptRunnerHelper.java:66)
        at org.apache.tools.ant.taskdefs.optional.Script.execute(Script.java:53)
        at org.apache.tools.ant.UnknownElement.execute(UnknownElement.java:293)
        at sun.reflect.GeneratedMethodAccessor4.invoke(Unknown Source)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at org.apache.tools.ant.dispatch.DispatchUtils.execute(DispatchUtils.java:106)
        at org.apache.tools.ant.Task.perform(Task.java:348)
        at org.apache.tools.ant.Target.execute(Target.java:435)
        at org.apache.tools.ant.Target.performTasks(Target.java:456)
        at org.codeaholics.tools.build.pant.AntWrapperImpl.executeTarget(Unknown Source)
        at org.codeaholics.tools.build.pant.DependencyGraphEntry.run(Unknown Source)
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at java.lang.Thread.run(Thread.java:748)
…hen running Ant with the Parallel-Ant executor.
@asfgit

This comment has been minimized.

Copy link

commented Dec 11, 2018

Can one of the admins verify this patch?

1 similar comment
@asfgit

This comment has been minimized.

Copy link

commented Dec 11, 2018

Can one of the admins verify this patch?

@mharmer

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2018

I have a test case I wrote as well, however I was hesitant on adding it since it dealt with non-deterministic behavior of trying to reproduce the race condition. If it's desired I can add it, it's basically this:

    /**
     * This test ensures that when requesting Project.getCopyOfReferences a
     * ConcurrentModificationException isn't thrown.
     *
     * ExecutorService is used so Exceptions are rethrown from their executors
     * and caught by JUnit as an error.
     */
    @Test
    public void testMultithreadedGetReferences() throws Exception {
        final ExecutorService es1 = Executors.newSingleThreadExecutor();
        final Future<?> getReferencesThread = es1.submit(() -> {
            for (int i = 0; i < 1000; i++) {
                p.getCopyOfReferences();
            }
        });

        final ExecutorService es2 = Executors.newSingleThreadExecutor();
        final Future<?> addReferencesThread = es2.submit(() -> {
            for (int i = 0; i < 1000; i++) {
                p.addReference("dummy" + i, "dummyValue");
            }
        });

        getReferencesThread.get();
        addReferencesThread.get();
    }
@jaikiran

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

this is ok to test

@jaikiran

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

I don't have enough knowledge about the thread safety aspects of the Project class. Stefan (@bodewig) would know more. However, having look at its code, I see similar locks and thread safety constructs for other members, so I think this change is fine.

As for the testcase, I think it's OK to add one. However, the snippet that you pasted might need some work. Right now, it uses a single thread and then in that thread's execution runs the APIs of the Project class in a loop. So it really doesn't exercise the multi-thread access behaviour and the access will all be sequential. What you could do is, use a fixed thread pool with a decent size (10 perhaps) and then use a loop (of 10) to submit the p.CopyOfReferences in that loop and later on (outside of that loop) get() the result for each of those Futures.

@asfgit

This comment has been minimized.

Copy link

commented Dec 12, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Linux/96/

@asfgit

This comment has been minimized.

Copy link

commented Dec 12, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Windows/102/

@mharmer

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

@jaikiran I think the test does exhibit the multithreaded scenario, I ran it prior to the Project changes to ensure it did fail. My Java is a bit rusty, but I believe the 2 separate ExecutorService's created run concurrently. My only concern is it's possible it may not catch the issue in the future as a regression test due to the non-determinism, as well as the somewhat arbitrary loop count and extended time taken to run the test (<100 milliseconds currently, but overall these types of tests tend to add up).

@jaikiran

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

@jaikiran I think the test does exhibit the multithreaded scenario, I ran it prior to the Project changes to ensure it did fail. My Java is a bit rusty, but I believe the 2 separate ExecutorService's created run concurrently.

You are right indeed. When I replied previously, I did not focus on the second executor submit and the fact that this task would run in parallel with the previous for loop that (might) still be in progress.

My only concern is it's possible it may not catch the issue in the future as a regression test due to the non-determinism, as well as the somewhat arbitrary loop count and extended time taken to run the test (<100 milliseconds currently, but overall these types of tests tend to add up).

Agreed. So it's fine to not include the test.

@jaikiran

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

@mharmer, what name would you like us to add to our contributors list (https://github.com/apache/ant/blob/master/CONTRIBUTORS) for your contribution?

@mharmer

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

@jaikiran: "Mark Harmer" will work, thanks.

@asfgit asfgit closed this in 41eb0d9 Dec 12, 2018
@avanishkant

This comment has been minimized.

Copy link

commented Aug 30, 2019

Which version of Ant is this fix going to be available in? I have taken 1.10.6 and the issue seems to be present that version as well.

@jaikiran

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

This fix is part of 1.10.6 release. If you are running into an issue, please start a discussion in our user mailing list that's listed on https://ant.apache.org/mail.html and provide the relevant details including the complete stacktrace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.