[NETSOCKET] TCPSocket & UDPSocket event handler cannot be run in IRQ context

[betzw]

Description

• Type: Bug
• Priority: Major

---

Bug

meed-os sha:
8f29517: Merge pull request #4317 from c1728p9/reduce_test_overhead
(i.e. current master)

Expected behavior
Sockets should not register callbacks (on state change events) which are not allowed to be executed in interrupt context.

Actual behavior
In method Socket::open() the virtual method Socket::event() gets registered as callback on state change of the socket. The API spec for NetworkStack::socket_attach() says:
The callback may be called in an interrupt context and should not perform expensive operations such as recv/send calls.
Unfortunately, both TCPSocket::event() and UDPSocket::event() call Semaphore::wait(0) which is not allowed to be called in interrupt context (following the CMSIS-RTOS specs for osSemaphoreWait()). This call consequently returns -1 rather than the number of available tokens causing (possibly erroneously) the release of the semaphore.

[0xc0170]

cc @kjbracey-arm @geky

[geky]

Good catch! Yeah, calling Semaphore::wait is an error in that context. We should remove it to avoid having code that doesn't do what it appears to. Also relying on error codes is probably just a bad idea.

However, while the Semaphore::wait call is incorrect, it's not going to break the network stack. Both of the TCP and UDP event semaphores are handled in a while loop like this:
https://github.com/ARMmbed/mbed-os/blob/master/features/netsocket/TCPSocket.cpp#L50-L76

The extra semaphore releases may waste cycles, but it shouldn't break anything.

At the moment there is an ongoing effort to bring over cmsis-rtos 2 (#4294), which will introduce bounded semaphores. A bounded semaphore would provide the correct behaviour that this logic is trying to accomplish and should be easy enough to switch to once the cmsis-rtos 2 pr is in.

[geky]

Note: this should be resolved by #4560

[betzw]

Well #4560 has been closed and not merged, so maybe it is not solved yet. Pls. correct me if I am wrong!

On the other hand, this one has become a real issue on my end, as the while loops you have indicated are actually failing to handle timeouts (correctly). This is just because, in the worst case, the extra semaphore releases might be called every time a character is received (in my case it is an ATParser based Wi-Fi module driver using the serial line as communication interface) and such outperform over the number of calls to Semaphore::wait().

[betzw]

Any progress on this?

cc @MarceloSalazar @screamerbg
This is currently a blocker for the SPWF01SA wifi driver!

[geky]

Sorry, the pr moved around a bit. I believe it's over here now: #4580

Although I think the wait call has already been removed from the socket class:
https://github.com/ARMmbed/mbed-os/blob/master/features/netsocket/TCPSocket.cpp#L191-L192

Are you running into an issue with the current master?

[betzw]

I am running into issues using mbed-os-5.5.x.
I have never tried the current master as I am in the middle of development and wanted a stable mbed-os beyond.

Anyway, I am afraid that https://github.com/ARMmbed/mbed-os/blob/master/features/netsocket/TCPSocket.cpp#L191-L192 will not resolve my issue as it does exactly what the previous version actually did, i.e. incrementing indistinctly the semaphores which in while loops like this might lead to timeouts being completely missed sunch ending up as endless loops.
This is because the stack operations (e.g. _stack->socket_connect() or _stack->socket_recv(), ecc.) called therein might cause the TCPSocket callback TCPSocket::event() being called at least one if not several times even if these operations might return an error. As a consequence count = _write_sem.wait(_timeout) will always lead to (count < 1) being false and therefor we will not break out and return from the loop ⚡

[betzw]

@nikapov-ST

[geky]

Ah yes, this should be fixed by #4580

[betzw]

Sounds good, once I have the time and/or all related PRs are merged I will give it a shot and let you know!

[betzw]

Hi again @geky,
Right in this moment I am trying out PR #4580 which in the meantime has been merged and am learning that not even this one is resolving my issue 🤕. This is again because the stack operations (e.g. _stack->socket_connect() or _stack->socket_recv(), ecc.) called therein might cause the TCPSocket callback TCPSocket::event() being called at least one if not several times even if these operations might return an error. The reason for this is, that in my implementation those operations require bi-directional communication with the WiFi module to be executed even if there is no data available in the module.
As a consequence _event_flag will always be set and therefor we will not break out and return from the loop ⚡
Thinking about it now, I am actually wondering if something like this can be solved at the NETSOCKET level at all or if it's not my duty to find a solution for this?
Your opinion on this is more than welcome!

[geky]

It should be fine for TCPSocket::event to get called extra times. We intentional support spurious events in the socket APIs.

This condition exits the function if socket_recv returned any code other than NSAPI_ERROR_WOULD_BLOCK. Following the code path in socket_recv, the _event_flag is only checked if socket_recv returns NSAPI_ERROR_WOULD_BLOCK.

So as long as socket_recv returns a negative error code or a positive count of bytes sent, the function should exit correctly. The only downside of _event_flag always being set is that the loop checking socket_recv will never sleep. Could it be you are always returning NSAPI_ERROR_WOULD_BLOCK?

Are multiple threads involved? It could be the socket_recv loop is starving your driver's thread. What priorities are your threads running at?