Non-atomic increment of volatile fields

[marschall]

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.

[v-nisidh]

Thanks @marschall for your inputs.

[xiangyushawn]

Thank you @marschall for reporting the issue. After investigation, we totally agree with your suggestions. However, we are working on a new multi-threading feature right now, We decide to mark this issue as enhancement and re-consider it in the future after the multi-threading feature is done. Thank you.

[marschall]

Yes @v-xiangs I agree it doesn't seem to be a correctness issue, just a bit confusing.

[xiangyushawn]

@marschall Thank you very much for the findings! We have created a PR #409 for this issue. Thanks!

[xiangyushawn]

Hello @marschall , thank you very much for your contribution. The PR is merged and we are closing this issue now.