Skip to content

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

Closed
gtomassi opened this issue Nov 14, 2020 · 3 comments
Closed

Uaysncio v3 Stream Writer Drain Bug #6621

gtomassi opened this issue Nov 14, 2020 · 3 comments

Comments

@gtomassi
Copy link

gtomassi commented Nov 14, 2020

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,

streamWriter = Stream(uart...
async def foo():
          streamWriter.write(packet)
          await streamWriter.drain()
for i in range(0,N):
          asyncio.create_task(foo())

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.

@gtomassi
Copy link
Author

@dpgeorge @peterhinch
This may be one to prioritize as it does immediately exhibit symptoms for anyone upgrading to v3. In particular it will likely happen when people queue up >1 of the legacy awrite's in succession.

If there is anything we can do to test a fix, we are happy to please let me know. Thank you.

@peterhinch
Copy link
Contributor

peterhinch commented Nov 15, 2020

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.

@jonnor
Copy link
Contributor

jonnor commented Sep 13, 2024

This seems to have been answered.

@jonnor jonnor closed this as completed Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants