-
Notifications
You must be signed in to change notification settings - Fork 465
Non-atomic increment of volatile fields #191
Description
There are two non-atomic increments of volatile fields in com.microsoft.sqlserver.jdbc.SocketFinder#noOfThreadsThatNotified and com.microsoft.sqlserver.jdbc.TDSWriter#packetNum. In Java the pre- and post-increment operators on volatile fields are not atomic. In Java volatile fields indicate that state is intended to be changed by multiple threads.
Since com.microsoft.sqlserver.jdbc.SocketFinder#noOfThreadsThatNotified is only used in updateResult and guarded by socketFinderlock I would remove volatile there.
com.microsoft.sqlserver.jdbc.TDSWriter#packetNum is a bit less obvious. The warning could simply be fixed by using either java.util.concurrent.atomic.AtomicInteger or java.util.concurrent.atomic.AtomicIntegerFieldUpdater, the latter has a bit lower overhead. On the other hand without knowing any details about the threading model it's hard to know if that is enough or even necessary. All the other fields changed in methods where packetNum is accessed seem to be non-volatile and the changes don't seem to be atomic. This suggests to me that those methods are not intended to be thread-safe and as a result removing volatile from packetNum would be my recommendation here as well.