Make RunNotifier code concurrent #625

kcooney · 2013-02-03T18:04:37Z

Allows multiple threads to concurrently send notifications to RunListeners. Existing RunListener implementations will continue to have their method calls synchronized. RunListener implementations annotated with net.jcip.annotations.ThreadSafe will have their method calls run in parallel if tests are run in parallel.

…ssues

kcooney · 2013-02-03T18:12:05Z

This is a copy of pull #578 with tests fixed and other cleanups applied.

I'm not yet happy with the state of the tests. We need better tests to verify that existing listener notifications are not called concurrently, but annotated implementations are called concurrently.

I'm not familiar with Maven, so I cannot verify if those changes are correct. Ideally, we should use the jcip-annotations jar at compile time, but users should not be required to have it at run-time.

I'm also not sure whether we should use jcip-annotations vs. our own annotation.

Tibor17 · 2013-02-04T00:01:41Z

@kcooney
I fixed your ignored tests. Now #addListener and #removeListener are backward compatible.
Maven ready as well.
See #578 .

dsaff · 2013-02-04T12:11:02Z

I'm planning to handle this pull request and the other with a light touch until @kcooney and @Tibor17 have iterated to their satisfaction. Please feel free to explicitly invoke me (with a ping to [email protected], if needed) if I should wake up sooner. Thanks!

Conflicts: build.xml src/main/java/org/junit/runner/notification/RunNotifier.java src/main/java/org/junit/runner/notification/SynchronizedRunListener.java src/test/java/org/junit/tests/AllTests.java

…02807be0344a9cf6acafa/src/test/java/org/junit/runner/notification/SynchronizedRunListenerTest.java

…882369652dfcadb3722b4/.classpath

Conflicts: src/test/java/org/junit/runner/notification/SynchronizedRunListenerTest.java src/test/java/org/junit/tests/AllTests.java

…_remove-jcip-deps Conflicts: src/test/java/org/junit/runner/notification/SynchronizedRunListenerTest.java

Update Javadoc Delete AddRemoveListenerTest (functionality covered by RunNotifierTest)

kcooney · 2013-02-14T18:50:02Z

@dsaff @Tibor17 I modified this pull request to match the proposal to have a new RunNotifier.ThreadSafe annotation and did a rewrite of the Javadoc. Please take another look

David, don't pull this via the git-hub UI; there are a lot of commits, and we don't want them all on the main branch. I'll do a non-fast-forward merge once everyone is comfortable with the final result.

Thanks for all the work!

Reduce diffs

dsaff · 2013-02-18T17:53:14Z

src/test/java/org/junit/runner/notification/SynchronizedRunListenerTest.java

+        Set<MethodSignature> synchronizedRunListenerMethods= getAllDeclaredMethods(
+                SynchronizedRunListener.class);
+
+        assertTrue(synchronizedRunListenerMethods.containsAll(runListenerMethods));


Clever. Thanks.

dsaff · 2013-02-18T18:03:25Z

@Tibor17, @kcooney, I've just gone through and made comments about the only places where I think we need to further make changes. There's not many.

@Tibor17, I gather that you feel that @kcooney has made more changes to your initial pull than strictly necessary. In some cases, I agree with @kcooney: the additional javadoc is quite helpful for implementors navigating the new thread-aware landscape we're setting before them. @kcooney appears to have already backed out the major unrelated changes that I saw on a previous commit. If in any place, I felt that the change was actually a negative move, I have indicated so. In any place where it appears to be a stylistic or naming preference issue, the simpler solution is to go with what we have now in this pull, rather than cherry-picking rollbacks.

Tibor17 · 2013-02-18T23:37:29Z

@kcooney
Check if jar of jcip files present.

Tibor17 · 2013-02-18T23:43:47Z

@dsaff
@kcooney
These are cosmetic changes. We need to go through the remarks in my comment which is right before @dsaff 's code inspection.

dsaff · 2013-02-19T18:07:35Z

@Tibor17, I did go through the remarks in that comment. I think we could go another couple of weeks debating each individual point--at this point, I'd like to see this get in, and then if you really feel that any of them have been missed that are truly bugs or missing features, I'll happily look at new pull requests fixing them. Like I said, in some cases, you did things in one way, and @kcooney did them in a different way that is equally valid, and I don't think reverting these one by one is productive.

kcooney · 2013-02-19T19:56:05Z

Thanks for the great feedback. I am traveling and had a poor WiFi
connection last night so I couldn't code. I'll try again tonight.

Tibor17 · 2013-02-19T22:56:11Z

This and next week i am busy with interviews, but i will try to connect to github if there's some spare time.

kcooney · 2013-02-21T05:59:27Z

@Tibor17

I am still working on some of the requested changes, but I wanted to respond with some questions and comments

I want to ask you to revert unrelated changes

I reverted the unrelated changes except for the trivial change to NOTICE.txt

New style: not to use f-prefixes.

I don't see that documented anywhere. I personally like the f-prefixes. If we change the style for fields, I think we should go back and fix all code under org/junit; a mix of styles for fields would be confusing.

you modified my tests and moved them to RunNotifierTest.

I think RunNotifierTest is a more logical place for these tests. The only changes were removing tests that used listeners that didn't obey the general contract of equals. I don't think we should be testing that, because the behavior can be undefined.

Do not use 3.x style on RunNotifierTest. Your debugging issue on your side is not problem of this pull.

See my question to David above.

SynchronizedRunListener#wrapIfNotThreadSafe must not be public, because SynchronizedRunListener is internal patch.

SynchronizedRunListener is a package-scope class, so there's no harm to making methods public. I like making the conceptually-public API of a class public; it makes it easier to see the API

keep using #wrapIfNotThreadSafe() in RunNotifier with private modifier as i did

I think where I have it is reasonable.

keep using my SynchronizedRunListener#equals() because of identical behavior with Listener#add and Listener#remove as it was before

If you can write a test using the public API of RunNotifer that fails with my version of equals but passes with yours, I will consider using your version. Until then, it's much better to have equals() implementations be symmetric, reflexive and transitive.

SynchronizedRunListener: Use simple synchronization. Simplicity, and modern VM perform fast any way

The prior RunNotifier synchronization would never call two RunListeners concurrently. So if I add two listeners that call common non-thread-safe code, everything will work fine with the old code and my code. With simple synchronization that won't be the case.

I'll update the JavaDoc of SynchronizedRunListener to make this clear

RunListener: Javadoc is not related to this problem
extensive Javadoc in RunNotifier should disappear.

I strongly disagree. Documenting the thread-safety guarantees should be part of this pull.

More Javadoc than really necessary would be problem to remove and maintain.

As you have pointed out before, I do tend to be verbose :-) Please feel free to comment on JavaDoc that isn't necessary.

Tests which test instanceof SynchronizedRunListener do not test behavior of entire RunNotifier and therefore do not present in SynchronizedRunListenerTest

Disagree. These are unit tests of SynchronizedRunListener. Both unit tests and integration tests can be useful. If there is a specific test that should be moved, please comment on the test file.

No need to annotate SynchronizedRunListener because the annotation has different purpose.

Why not? SynchronizedRunListener is thread safe, after all. It also allows us to have two examples of thread-safe listeners in the code base.

kcooney · 2013-02-26T01:15:23Z

Okay, made a bunch of changes. Please take another look

dsaff · 2013-02-27T22:33:06Z

src/main/java/org/junit/runner/notification/RunNotifier.java

    }

    /**
     * Internal use only
     */
    public void removeListener(RunListener listener) {
-        fListeners.remove(listener);
+        if (listener == null) {


Just move this check to wrapIfNotThreadSafe?

I could do that, but then the NullPointerException wouldn't have a message. If that's fine with you, I can make the change.

Ah, missed the extra exception info. Never mind, carry on.

dsaff · 2013-02-27T22:36:26Z

Looks to me to be iterating toward completion. A couple more (smaller) comments.

kcooney · 2013-03-01T06:38:08Z

@dsaff PTAL

Tibor17 · 2013-03-01T18:50:47Z

I prefer SynchronizedRunListener#wrapIfNotThreadSafe moving to RunNotifier as private method, because the user can implement thread-safe listener without using locks.
Using the lock is a big hammer and in some cases I can implement my own listener thread-safe without any locks.
Notice that we used locks only because user's internal impl of listener is unknown for us.
Nevertheless unsafe constructor is not treated. The delegated listener may have unsafe constructor.
So pls do not force the user to call SynchronizedRunListener#wrapIfNotThreadSafe as a treatment. Suppose the user is wise to implement thread-safety with/out blockers/lock as he recognized by his implementation details.

kcooney · 2013-03-02T08:31:35Z

@Tibor17 I don't understand the issue. If the user implements a thread-safe listener without locks, then they should just annotate the listener class with @RunListener.ThreadSafe and RunNotifier won't wrap the listener. If the user passes in a RunListener that had an unsafe constructor then they are already broken; I don't see how these changes make it worse.

The end-user isn't calling SynchronizedRunListener.wrapIfNotThreadSafe(); it's a method on a package-scope class.

Tibor17 · 2013-03-02T09:29:25Z

The method #wrapIfNotThreadSafe method is public. It provides a listener for RunNotifier. So I do not see reason why this method should be out of the notifier.

I have another problem with the static monitor in SynchronizedRunListener. It's a common lock across multiple JunitCore instances/RunNotifiers. Since you wanted prevent from overlapping listeners across multiple threads, the static lock is even worse than the ment treatment.
Let's think of lock per RunNotifier like with this constructor:
SynchronizedRunListener(Object lock) { ... }
Then in RunNotifier call in add/removeListener:
RunListener listener = wrapIfNotThreadSafe(nonStaticNotifiersLock); // not necessarily this pointer of RN collection.add/remove(listener);

kcooney · 2013-03-02T17:25:09Z

@Tibor17 Done.

Tibor17 · 2013-03-05T21:15:25Z

@dsaff
I do not have any biggest objections now.

dsaff · 2013-03-05T23:57:39Z

All right, I think we're good to go. Thank you very much to both of you for iterating to a great finishing point. I'll work with @kcooney to merge it in cleanly.

dsaff · 2013-03-06T15:35:16Z

Merged! Thanks, all.

Tibor Digana and others added 10 commits December 19, 2012 19:41

synchronized substituted by thread safe collection

4df3e81

1E3 -> 1000, 1E2 -> 100

d2f1710

#realUsage test, simplified

a691962

Merge branch 'master' of git://github.com/KentBeck/junit into junit.i…

497e6ca

…ssues

AllTests

573828c

Merge branch 'master' of git://github.com/KentBeck/junit into junit.i…

20c6b53

…ssues

problem to remove synchronized listener

21a926e

backward compatible, SynchronizedRunListener, removed EqualRunListener

1d6914b

Fix equals method for SynchronizedRunListener

c099a99

Cleanup implementation and tests

41626fd

kcooney mentioned this pull request Feb 3, 2013

Improvement: Concurrent Queue in RunNotifier instead of Old Synchronized ArrayList #578

Closed

using COWAL + backward compatible + SynchronizedRunListener

c220ad7

kcooney and others added 13 commits February 4, 2013 13:07

Merge branch 'Tibor17-junit.issues' into concurrent-run-listeners

5112c1a

Conflicts: build.xml src/main/java/org/junit/runner/notification/RunNotifier.java src/main/java/org/junit/runner/notification/SynchronizedRunListener.java src/test/java/org/junit/tests/AllTests.java

Add tests in AddRemoveTestListenerTest to RunNotifierTest

a04aae1

merged from https://github.com/kcooney/junit/blob/41626fd3022c0d73d6f…

a5e8a71

…02807be0344a9cf6acafa/src/test/java/org/junit/runner/notification/SynchronizedRunListenerTest.java

merged from https://github.com/kcooney/junit/blob/c099a99b76ccd985639…

f8016a4

…882369652dfcadb3722b4/.classpath

clarified the purpose of SynchronizedRunListener

0c35851

AllTests run SynchronizedRunListenerTest

e30fe38

Remove jcip-annotations dependency; add @Concurrent annotation

796801e

threadsafe annotation

708d972

Merge branch 'Tibor17-junit.issues' into concurrent-run-listeners

02f1486

Conflicts: src/test/java/org/junit/runner/notification/SynchronizedRunListenerTest.java src/test/java/org/junit/tests/AllTests.java

Remove tabs

789c302

Merge branch 'concurrent-run-listeners' into concurrent-run-listeners…

cc7d45b

…_remove-jcip-deps Conflicts: src/test/java/org/junit/runner/notification/SynchronizedRunListenerTest.java

Remove dependency on jcip-annotations

58a5a92

Replace Concurrent annotation with RunNotifier.ThreadSafe

2873269

Update Javadoc Delete AddRemoveListenerTest (functionality covered by RunNotifierTest)

Remove mention of jcip-annotations from NOTICE.txt

10c5130

Reduce diffs

dsaff reviewed Feb 18, 2013
View reviewed changes

kcooney added 4 commits February 24, 2013 17:44

Make RunNotifierTest a JUnit4-style test again

a6fb342

Update in response to code review commends by dsaff and Tibor17

4b8efa1

Make code style consistent

a85566f

Undo changes to notice

b11b5aa

dsaff reviewed Feb 27, 2013
View reviewed changes

kcooney added 2 commits February 28, 2013 22:13

minor cleanup

e1172af

Revert changes to build.xml

38ac72e

Move wrapIfNotThreadSafe() to RunNotifier

a400a3a

dsaff closed this Mar 6, 2013

kcooney deleted the concurrent-run-listeners branch January 6, 2017 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make RunNotifier code concurrent #625

Make RunNotifier code concurrent #625

kcooney commented Feb 3, 2013

kcooney commented Feb 3, 2013

Tibor17 commented Feb 4, 2013

dsaff commented Feb 4, 2013

kcooney commented Feb 14, 2013

dsaff Feb 18, 2013

dsaff commented Feb 18, 2013

Tibor17 commented Feb 18, 2013

Tibor17 commented Feb 18, 2013

dsaff commented Feb 19, 2013

kcooney commented Feb 19, 2013

Tibor17 commented Feb 19, 2013

kcooney commented Feb 21, 2013

kcooney commented Feb 26, 2013

dsaff Feb 27, 2013

kcooney Mar 1, 2013

dsaff Mar 5, 2013

dsaff commented Feb 27, 2013

kcooney commented Mar 1, 2013

Tibor17 commented Mar 1, 2013

kcooney commented Mar 2, 2013

Tibor17 commented Mar 2, 2013

kcooney commented Mar 2, 2013

Tibor17 commented Mar 5, 2013

dsaff commented Mar 5, 2013

dsaff commented Mar 6, 2013

Make RunNotifier code concurrent #625

Make RunNotifier code concurrent #625

Conversation

kcooney commented Feb 3, 2013

kcooney commented Feb 3, 2013

Tibor17 commented Feb 4, 2013

dsaff commented Feb 4, 2013

kcooney commented Feb 14, 2013

dsaff Feb 18, 2013

Choose a reason for hiding this comment

dsaff commented Feb 18, 2013

Tibor17 commented Feb 18, 2013

Tibor17 commented Feb 18, 2013

dsaff commented Feb 19, 2013

kcooney commented Feb 19, 2013

Tibor17 commented Feb 19, 2013

kcooney commented Feb 21, 2013

kcooney commented Feb 26, 2013

dsaff Feb 27, 2013

Choose a reason for hiding this comment

kcooney Mar 1, 2013

Choose a reason for hiding this comment

dsaff Mar 5, 2013

Choose a reason for hiding this comment

dsaff commented Feb 27, 2013

kcooney commented Mar 1, 2013

Tibor17 commented Mar 1, 2013

kcooney commented Mar 2, 2013

Tibor17 commented Mar 2, 2013

kcooney commented Mar 2, 2013

Tibor17 commented Mar 5, 2013

dsaff commented Mar 5, 2013

dsaff commented Mar 6, 2013