Skip to content
This repository has been archived by the owner on Jul 7, 2020. It is now read-only.

squid:S1860 - Synchronization should not be based on Strings or boxed primitives #199

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

georgekankava
Copy link

This pull request is focused on resolving occurrences of Sonar rule
squid:S1860 - Synchronization should not be based on Strings or boxed primitives.
You can find more information about the issue here:
https://dev.eclipse.org/sonar/coding_rules#q=squid%3AS1860
Please let me know if you have any questions.
George Kankava

@tea-dragon
Copy link
Contributor

the change to ValueFilterRandom seems safe enough, but it seems like you have replaced instance level locking with class (static) level locking (maybe that is fine anyway. It isn't a hugely important filter...). However, in FactoryInputStream, there are several calls to eg. key.wait() and key.notifyAll() which are not okay with the removal of the synchronization on them on the key object. see eg.: https://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#wait().

Those are both pretty bizarre classes though. I think i'd end up just gutting most of them myself. As an aside, I generally prefer to go with explicit locks over synchronization blocks. This isn't widely accepted as the best practice or anything, but in my experience, synchronized keywords on methods, blocks, "this", and in static methods tend to lead to all kinds of shenanigans.

Sorry I haven't gotten back to your previous PR yet, but thanks for your continued efforts! :)

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

Successfully merging this pull request may close these issues.

3 participants