goxr3plus / java-stream-player

šŸŒŒJava Advanced Audio Controller Library (WAV, AU, AIFF, MP3, OGG VORBIS, FLAC, MONKEY's AUDIO and SPEEX audio formats )
GNU General Public License v3.0
147 stars 33 forks source link

Questionable redefinition of audioInputStream #55

Open HelgeStenstrom opened 4 years ago

HelgeStenstrom commented 4 years ago

This looks a bit weird in my eyes

https://github.com/goxr3plus/java-stream-player/blob/6a00a79f32bf324f7d6232182d93745da64bee0b/src/main/java/com/goxr3plus/streamplayer/stream/StreamPlayer.java#L506-L507

audioInputStream is a field which is given a value in initAudioInputStream() https://github.com/goxr3plus/java-stream-player/blob/6a00a79f32bf324f7d6232182d93745da64bee0b/src/main/java/com/goxr3plus/streamplayer/stream/StreamPlayer.java#L330-L331

Then it is redefined, i.e., it loses its old value and get a new one. Why isn't a different variable (field) used for the new value?

There is another issue with the assignment on line 507:

Non-atomic operation on volatile field 'audioInputStream' Inspection info: Reports any non-atomic operations on volatile fields. Non-atomic operations on volatile fields are operations where the field is read and the value is used to update the field. It is possible for the value of the field to change between the read and the write, possibly invalidating the operation. The non-atomic operation can be avoided by surrounding it with a synchronized block or by making use of one of the classes from the java.util.concurrent.atomic package.

As mentioned earlier, it's probably a mistake to make audioInputStream volatile. SonarQube issues a warning where the volatile field is declared. For a description, see https://wiki.sei.cmu.edu/confluence/display/java/CON50-J.+Do+not+assume+that+declaring+a+reference+volatile+guarantees+safe+publication+of+the+members+of+the+referenced+object