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

Use Vector for EventHandler #4028

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
4 participants
@eatkins
Copy link
Contributor

commented Mar 21, 2018

ArrayList::add is not thread safe. I ran into cases where async tests
using utests would fail even when all of the individual tests passed.
This was because multiple threads called back into the handle method of
the handler instance variable, which just delegated to eventList::add.
When this happened, one of the events would get added to the list as a
null reference, which would manifest as an NPE upstream on the master
process. After this change, my tests stopped failing.

(See the guidelines for contributing, linked above)

@eed3si9n eed3si9n added the ready label Mar 21, 2018

@dwijnand
Copy link
Member

left a comment

lgtm! nice fix, @eatkins!

two thoughts:

  1. I think we could merge this into 1.1.x too for 1.1.2
  2. would it be possible to construct a test for this? (e.g with async tests in utest)
@dwijnand

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

/cc @retronym who's also been dealing with concurrency issues in sbt's testing.

@retronym

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

Yep, #3997 addresses a similar problem in non-forked tests.

@retronym

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

But I also recently discovered #4010 in the context of Scalacheck forked tests, which has the same symptom you're reporting: a single test failure doesn't fail the entire suite. That wasn't a race condition, but a problem of losing data by clobberring a binding in a map of results, when the test framework created many nested Tasks for fine grained parallel execution (each test methods in a single class got a parallel Task, but because these shared the fully-qualified-name of the enclosing class the result aggregation code only ended up using the status of the final test)

@eatkins How sure are you that this change (alone) fixes the problem you saw?

@@ -294,7 +295,7 @@ private void runTestTasks(final ExecutorService executor, final Task[] tasks, fi
Task[] nestedTasks;
final TaskDef taskDef = task.taskDef();
try {
final List<ForkEvent> eventList = new ArrayList<ForkEvent>();

This comment has been minimized.

Copy link
@retronym

retronym Mar 21, 2018

Member

Alternatively:

final Collection<ForkEvent> eventList = new java.util.concurrent.ConcurrentLinkedDeque<ForkEvent>();

This comment has been minimized.

Copy link
@eatkins

eatkins Mar 21, 2018

Author Contributor

I like this if for no other reason than that it makes it very explicit that we expect that the eventList may be touched by multiple threads.

@@ -294,7 +295,7 @@ private void runTestTasks(final ExecutorService executor, final Task[] tasks, fi
Task[] nestedTasks;
final TaskDef taskDef = task.taskDef();
try {
final List<ForkEvent> eventList = new ArrayList<ForkEvent>();

This comment has been minimized.

Copy link
@retronym

retronym Mar 21, 2018

Member

I agree this is racy.

@eatkins eatkins force-pushed the eatkins:vector branch from 180ae16 to f2661b5 Mar 21, 2018

@eatkins

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2018

I added a test to the PR that induces the data race that I was seeing.

@retronym I think my issue was slightly different than #4010. In my case, all of my individual tests passed, but the overall suite was failing.

@eatkins eatkins force-pushed the eatkins:vector branch from f2661b5 to 684cc73 Mar 21, 2018

Ethan Atkins
Add test for async utest TestSuites
Sometimes when utest runs async tests (i.e. tests that return a future)
the test suite will fail even when none of the individual tests do. This
is due to a data race in sbt. Most, but not all, of the time, this test
will induce that race.

@eatkins eatkins force-pushed the eatkins:vector branch from 684cc73 to b6e3390 Mar 21, 2018

Ethan Atkins
Use ConcurrentLinkedDeque for EventHandler
ArrayList::add is not thread safe. I ran into cases where async tests
using utests would fail even when all of the individual tests passed.
This was because multiple threads called back into the handle method of
the handler instance variable, which just delegated to eventList::add.
When this happened, one of the events would get added to the list as a
null reference, which would manifest as an NPE upstream on the master
process. After this change, my tests stopped failing.
@eed3si9n
Copy link
Member

left a comment

LGTM
I'd also vote to rebase this to 1.1.x.

@eed3si9n

This comment has been minimized.

Copy link
Member

commented Mar 22, 2018

It seems #4030 does just that.

@dwijnand

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

closing in favour of #4030

@dwijnand dwijnand closed this Mar 23, 2018

@dwijnand dwijnand removed the ready label Mar 23, 2018

@eatkins eatkins deleted the eatkins:vector branch Oct 9, 2018

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