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 some thread safety and exception-related problems #195

Merged
merged 3 commits into from
Aug 28, 2022

Conversation

A248
Copy link
Contributor

@A248 A248 commented Aug 25, 2022

  1. Regarding EconomyProvider, the previous behavior returned the Set early, before the computations inside the loop necessarily complete. Also, the Set was unsynchronized despite usage in a multithreaded context. Thirdly, Subscriber#succeed was called regardless of failure; now, #succeed is not called if an exception occurs.
  2. Using ExecutorService#submit implies the caller will analyze the returned Future for exceptions. While your usages are not intended to throw exceptions, future code refactoring and extreme cases can turn debugging missed exceptions into a hellish experience. Using #execute ensures exceptions will at the least be logged.
  3. To keep EventBus thread-safe, EventTypeTracker ought likewise to be thread-safe. Thus the friends map should be concurrent and atomics performed on it atomic. Additionally, I've simplified the code in the process and removed an unnecessary collect-and-stream operation.

I apologize for not strictly following the contribution process: I became carried away and simply completed all this, before reading the guidelines. However, I think you will find that "people say it sounds good."

A248 added 3 commits August 25, 2022 13:45
It should not happen, but in the rare case it does, at least an
  exception will be logged.
Remove unnecessary collect() and re-stream() operation
@lokka30 lokka30 requested a review from MrIvanPlays August 26, 2022 01:05
@lokka30
Copy link
Member

lokka30 commented Aug 26, 2022

Thank you very much for the pull request A2, I will be sure to review it as soon as I get some free time. :)

@lokka30 lokka30 merged commit 1605815 into ArcanePlugins:master Aug 28, 2022
@Geolykt
Copy link
Contributor

Geolykt commented Aug 28, 2022

Incredibly tiny nitpick (doesn't really have an effect as at that point it is all single-threaded) I'd prefer using ConcurrentHashMap#newKeySet() over using a standard synchronized HashSet as you cannot modify them while iterating over synchronized Set.

Or use an unmodifiable set, which might be the better option considering that I do not think that it should be modified at that point onwards

@A248
Copy link
Contributor Author

A248 commented Aug 29, 2022

Replying to @Geolykt :

Since mutability of the returned Set is not guaranteed by the method contract, I don't think clients should be assuming mutability -- and surely it seems too much to assume mutability within the context of iteration, which is generally a rare property of Collection implementations.

However, I like your idea of using an unmodifiable set. A documentation PR clarifying the status of the returned Set would be useful here: i.e., have the javadocs specify the collection yielded may or may not be mutable. Furthermore, these are default methods; the provider's implementation can change, and so may ours (see #180).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants