[core] Allow for thread-safe report listeners #193
Conversation
- This follows discussion on pmd#125 (comment) - We add a marker interface for thread-safe listeners, and implement it were appropriate. - Breaking change: `getSynchronizedListeners` and `addSynchronizedListeners` now handle `List<ThreadSafeReportListener>`. - We may mark the non-thread-safe methods and interfaces as deprecated and force everyone to handle synchronization on their own for future releases.
jsotuyod
added
an:enhancement
| @@ -512,7 +522,7 @@ public long getElapsedTimeInMillis() { | |||
| return end - start; | |||
| } | |||
|
|
|||
| public List<SynchronizedReportListener> getSynchronizedListeners() { | |||
| public List<ThreadSafeReportListener> getSynchronizedListeners() { | |||
There was a problem hiding this comment.
I was not sure, since I'm already breaking the API, if I should rename this method right now or not. What do you think @adangel ?
There was a problem hiding this comment.
I'd say, rename the method right away.
I think, it's ok to break the API since we are on the master (5.6.x) branch only.
However, we should have a short notice in the changelog under section "API Changes" - just in case.
I assume, integration/upgrading of PMD with other tools, usually works like this: Upgrade PMD dependency and if it compiles and tests successfully -> great. If not, maybe look at the changelog or just read the code...
jsotuyod
added
the
for:performance
| @@ -512,7 +522,7 @@ public long getElapsedTimeInMillis() { | |||
| return end - start; | |||
| } | |||
|
|
|||
| public List<SynchronizedReportListener> getSynchronizedListeners() { | |||
| public List<ThreadSafeReportListener> getSynchronizedListeners() { | |||
There was a problem hiding this comment.
I'd say, rename the method right away.
I think, it's ok to break the API since we are on the master (5.6.x) branch only.
However, we should have a short notice in the changelog under section "API Changes" - just in case.
I assume, integration/upgrading of PMD with other tools, usually works like this: Upgrade PMD dependency and if it compiles and tests successfully -> great. If not, maybe look at the changelog or just read the code...
| * extra synchronization. | ||
| * | ||
| * Thread-safety is required only for concurrently notifying about different files. | ||
| * Same file violations are guaranteed to be reported serially. |
| @@ -9,7 +9,7 @@ | |||
| /** | |||
| * Wraps a report listener in order to synchronize calls to it. | |||
| */ | |||
| public final class SynchronizedReportListener implements ReportListener { | |||
| public final class SynchronizedReportListener implements ThreadSafeReportListener { | |||
There was a problem hiding this comment.
Do we still need this SynchronizedReportListener? I think, PMD itself doesn't use it directly anymore with your changes... so we could delete it (or deprecate it)
Or should we provide it as an example of a very simple, but possibly too much synchronizing listener, e.g. as a help to migrate your existing listener to a ThreadSafeReportListener?
- Breaking change to public API to retrieve listeners.
|
@adangel there you go! I renamed the method, and deprecated both the old We should definitely add proper warnings in the changelog when we merge. |
|
Thanks! |
getSynchronizedListenersandaddSynchronizedListenersnow handleList<ThreadSafeReportListener>.to handle synchronization on their own for future releases.