-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
Uaysncio v3 Stream Writer Drain Bug #6621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
@dpgeorge @peterhinch If there is anything we can do to test a fix, we are happy to please let me know. Thank you. |
This is not a safe way to use a StreamWriter. At a hardware level a stream device may buffer C characters before temporarily stalling while these are physically transmitted. Assume C << len(packet). The currently transmitting task sends a batch of up to C characters, the hardware signals "full" via the ioctl and the task yields to the scheduler. When the hardware has space in its buffer the ioctl allows one of the tasks waiting on the StreamWriter to send a batch of characters. It won't be the task which sent the last batch. The result will be garbled messages. The correct way to use StreamWriter with multiple tasks is to implement a queue. Multiple tasks put messages on the queue, and a single task removes them and issues them to the StreamWriter. No other task accesses the StreamWriter. Note that this logic applies to most approaches to asynchronous handling of streams and is not tied to the specific implementation of uasyncio. |
This seems to have been answered. |
The new implementation of Stream will write redundant frames out it's socket/serial connection if used asynchronously.
Observed on the STM32 UART.
Expected/Actual Behavior & Steps to replicate:
This pseudo-code should write N packets out the uart,
but instead will likely write the first packet N times, the second packet N-1 times, ... the last packet one time (not in that order).
Root cause:
In the drain method of Stream, it pauses at the yield and allows the other coroutines to start, initiating N drain operations where the variable 'off' is 0 in each, and thus the entire buffer is written for each operation. Thus the drain method is not "thread safe" or idempotent in the sense that the 'off' variable is not independent of other concurrent operations; it is thus susceptible to race conditions.
Proposed Fix:
The drain operation should be idempotent, either maintaining some sort of stateful buffer reading in the Stream instance, or limiting itself to processing only one write out loop at a time.
Temporary Workaround:
Comment out the yield operation in drain (
yield core._io_queue.queue_write(self.s)
). This was tested and seems to be working, but wouldn't consider it safe or reliable since I'm not sure how the entire IO queuing process works and if it's safe to disable.The text was updated successfully, but these errors were encountered: