socket_sigio test uses shared equeue and thread #5527
Conversation
|
Based on the referenced PR, this looks good to me. Waiting for the reviewers feedback |
|
This does actually raise a number of issues, but they're quite involved. Most notably, the socket using the sigio and being read on the event queue is actually blocking. The fact it's blocking is covering up some invalid use of sigio - it blocks on the first sigio until data is actually available (which may not be until a later sigio), and the fact that it's was a private event queue covered up the fact that it's blocking. Plus there's an invalid assumption that all the data arrives in a single read (although it's probably true for a "hello world"). So this would incorrectly block the global event queue, and could theoretically cause deadlock if the network stack were also trying to use it (similar sort of deadlock we see trying to do DNS from mbed client - mbed client and Nanostack are sharing an event queue, so mbed client doing blocking DNS jams the stack). I'd suggest not going for the global queue unless you want to do a rework to make it properly non-blocking, and then fixing up the sigio handler to cope with that - make it accumulate data into the buffer on each sigio, and finish when it reads 0 (the end of stream marker), rather than just trigger the completion on the first sigio. An alternative simpler change to purely fix the thread lifetime issues without starting to pick at the network bits would just to be to use a private static thread+event queue in your test - it doesn't matter if you block your own private event queue. |
|
@sarahmarshy What do you think about the latest comment? |
|
@sarahmarshy Please rebase this and address Kevin's comments. |
Description
This modifies the socket_sigio test to use a shared EventQueue and Thread between test cases. Previously, the EventQueue and Thread were created on the stack for each test case.
This is implemented based on feedback from @kjbracey-arm and @pan- in #5474
Status
READY