Sequence numbers are generated by incrementing the static int "org.smpp.pdu.PDU.sequenceNumber". This operation is not guarded by a lock. This makes it unsafe to concurrently use more than one SMPP session.
Reads and writes to/from a static primitive (other than a long or a
double) are atomic. So this operation is safe:
setSequenceNumber(++sequenceNumber);
The increment will be atomic, and multiple threads executing this code
section in quick succession should each reliably get their own local
copy of a brand new sequence number to pass to setSequenceNumber().
The static int sequenceNumber doesn't seem to have any static dependents
either, so that looks fine.
The only potentially problematic issue I see is this:
// it's likely that this will be the assigned seq number
// (in simple cases :-)
dbgs += "[" + (sequenceNumber+1) + "]";
which could generate an inaccurate log entry, given the right
circumstances - but, those circumstances look pretty unlikely to happen
in practice; anyway, only the debug log entry would be incorrect, but
not the actual sequence number.
Let me know if you see something that I missed.
Bye
Paolo
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The increment operation, ++sequenceNumber, is not atomic, as it represents three atomic operations - getting the value, adding one to it, and setting it - so there is a potential race condition. The only safe approach, unless you're using JDK1.5, is to synchronize access to sequenceNumber. The additional overhead of synchronizing access to this variable is insignificant.
-Dave
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The ++sequenceNumber would be threadsafe if this int variable would be volatile.
In this case, the variable wasn't marked volatile and the JVM sometimes used the cached value of the variable. This is now, with the current model but it is safer to protect with synchronize. for better performance the client code could use an AtomicInteger and set the PDU sequence number manually
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
This has already been fixed in SCM by synchronizing (volatile alone would not have been enough to safeguard ++ operation on an int). Can't use AtomicInteger until we commit to >= JDK1.5. Closing comment posting on this closed bug.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Logged In: YES
user_id=1108378
Originator: NO
Hi David
Reads and writes to/from a static primitive (other than a long or a
double) are atomic. So this operation is safe:
setSequenceNumber(++sequenceNumber);
The increment will be atomic, and multiple threads executing this code
section in quick succession should each reliably get their own local
copy of a brand new sequence number to pass to setSequenceNumber().
The static int sequenceNumber doesn't seem to have any static dependents
either, so that looks fine.
The only potentially problematic issue I see is this:
// it's likely that this will be the assigned seq number
// (in simple cases :-)
dbgs += "[" + (sequenceNumber+1) + "]";
which could generate an inaccurate log entry, given the right
circumstances - but, those circumstances look pretty unlikely to happen
in practice; anyway, only the debug log entry would be incorrect, but
not the actual sequence number.
Let me know if you see something that I missed.
Bye
Paolo
Logged In: YES
user_id=1654118
Originator: YES
Hi,
The increment operation, ++sequenceNumber, is not atomic, as it represents three atomic operations - getting the value, adding one to it, and setting it - so there is a potential race condition. The only safe approach, unless you're using JDK1.5, is to synchronize access to sequenceNumber. The additional overhead of synchronizing access to this variable is insignificant.
-Dave
Logged In: YES
user_id=1108378
Originator: NO
Hi Dave
My stupidity, I see you're right. Change is in CVS.
Thanks
Paolo
The ++sequenceNumber would be threadsafe if this int variable would be volatile.
In this case, the variable wasn't marked volatile and the JVM sometimes used the cached value of the variable. This is now, with the current model but it is safer to protect with synchronize. for better performance the client code could use an AtomicInteger and set the PDU sequence number manually
This has already been fixed in SCM by synchronizing (volatile alone would not have been enough to safeguard ++ operation on an int). Can't use AtomicInteger until we commit to >= JDK1.5. Closing comment posting on this closed bug.