mqtt-streaming: Prefer wrapping instead of reissuing packet ids #1489
Conversation
| private val MinPacketId = PacketId(1) | ||
| private val MaxPacketId = PacketId(0xffff) | ||
| val MinPacketId = PacketId(1) | ||
| val MaxPacketId = PacketId(0xffff) |
There was a problem hiding this comment.
These are enclosed in a private[streaming] already. I am using them in the tests, hence removing the private here.
longshorej
force-pushed
the
mqtt-packet-ids
branch
from
35e3896
to
5e25a1e
Compare
|
I’m wondering if we can simplify this even further. The idea is centered on maintaining a set where we track the allocated packet ids.
When deallocating, simply remove from the set. |
longshorej
force-pushed
the
mqtt-packet-ids
branch
from
5e25a1e
to
e66f5c3
Compare
| // all packet ids are taken, so we'll wait until one is unregistered | ||
| // to continue | ||
|
|
||
| main(registrantsByPacketId, nextPacketId, pendingRegistrations :+ register) |
There was a problem hiding this comment.
I'm still unsure as to the benefit of having a pendingRegistrations structure. It should be a rare condition and adds complexity.
|
@ennru, @longshorej and I discussed further - this PR is now reviewed from our perspective and ready to go. |
Packet ids now monotonically increase until they reach the maximum value, upon which they wrap around to the minimum. Care is taken to ensure that packet ids that are in use are not issued, as it is technically possible, albeit rare. This fixes a number of potential race conditions. For example, consider a PUBLISH -> PUBACK exchange with QoS1 with the old behavior: 1) Server retransmits an unacknowleged publish 2) Before client receives retransmission, it finally ACKs original publish. 3) Server receives ACK, and performs new publish, reusing the last id. 4) Client receives retransmission from #1, and ACKs it. 5) Server receives ACK, and misinterprets it as an ack from #3 6) Oh no! By wrapping around, we've effectively eliminated the changes of this happening. This is similar to how Linux issues PIDs for processes to avoid a similar sort of problem.
longshorej
force-pushed
the
mqtt-packet-ids
branch
from
e66f5c3
to
2dea455
Compare
|
Rebased! |
Packet ids now monotonically increase until they reach the maximum value, upon which they wrap around to the minimum. Care is taken to ensure that packet ids that are in use are not issued, as it is
technically possible, albeit rare.
This fixes a number of potential race conditions. For example, consider a PUBLISH -> PUBACK exchange with QoS1 with the old behavior:
By wrapping around, we've effectively eliminated the changes of this happening. This is similar to how Linux issues PIDs for processes to avoid a similar sort of problem.
/cc @huntc