Protect transportWrite with writeMutex#43
Conversation
Emil-Juhl
requested review from
bogofki,
hansbinderup,
hjespersen,
jeppefrandsen and
tomthuneby
Emil-Juhl
force-pushed
the
emdj/lock-in-writeFrame
branch
from
c01de16 to
4ca08ae
Compare
|
@jeppefrandsen thanks for the review! After some discussion with @tomthuneby we decided on introducing a new mutex instead. Our client code may be calling into hdlcpp->write from multiple threads and others might be doing the same. I think my previous approach was maybe a bit too heavy handed with regards to changing the semantics of Hdlcpp::write with respect to the thread_safety around sequence number logic etc. |
I also think this is more safe. Did not check the current client side code (only on mobile 😅) |
Guard usage of the `transportWrite` and encoding frames into the `writeBuffer` by adding a `writeFrameMutex` to be locked within `writeFrame`. The `write` function relies on repeated calls to `read` that need to be performed by a separate thread from the user of hdlcpp. This is because `write` will block until a `frameAck` or `frameNack` is received or a timeout is hit. The hdlcpp implementation, however, also performs writes on the transport layer from within the `read` function itself. It does so in the process of either sending a `frameAck` or `frameNack` upon receival of a data frame. From this it is aparent that any user of hdlcpp is basically forced into a race condition that can potentially mangle frames on the wire. The race can be hit by simply receiving data frames while trying to transmit data frames as well. Since the function which actually encodes data into the `writeBuffer` and calls the `transportWrite` callback is `writeFrame`, adding a mutex to specifically guard this function can prevent the race. This makes sure that if the thread handling `read` were to attempt transmitting a `frameAck` while another thread was attempting to transmit a data frame, then these two frames don't get mixed together. By introducing a new mutex rather than just moving the existing `writeMutex`, the thread safety of the sequence numbering increments within `write` is kept as-is.
Emil-Juhl
force-pushed
the
emdj/lock-in-writeFrame
branch
from
4ca08ae to
00793dd
Compare
3 participants
Guard usage of the
transportWriteand encoding frames into thewriteBufferby locking thewriteMutexwithinwriteFramerather than only withinwrite.The
writefunction relies on repeated call toreadthat need to be performed by a separate thread from the user of hdlcpp. This is becausewritewill block until aframeAckorframeNackis received or a timeout is hit.The hdlcpp implementation, however, also performs writes on the transport layer from within the
readfunction itself. It does so in the process of either sending aframeAckorframeNackupon receival of a data frame.From this it is aparent that any user of hdlcpp is basically forced into a race condition that can potentially mangle frames on the wire. The race can be hit by simply receiving data frames while trying to transmit data frames as well.
Since the function which actually encodes data into the
writeBufferand calls thetransportWritecallback iswriteFramethewritemutex should be locked here and not on thewritefunction. This makes sure that if the thread handlingreadwere to attempt transmitting aframeAckwhile another thread was attempting to transmit a data frame, then these two frames don't get mixed together.Of course, one can argue that now the
writeAPI is not "thread safe" since it doesn't have its own distinct mutex. However, it should be fair to place the burden of thread safety for calls intowriteon the user of the respective hdlcpp instance.