extmod/modlwip: Fixed socket poll in reset state

[rkojedzinszky]

Fixes #3449

[dpgeorge]

Thanks for the patch but in this case I think the best thing to do is make the smallest change to fix the issue. This will most likely be the change that increases the code size by the least, and doesn't affect any other behaviour. The minimal change would be to simply check that socket->pcb.tcp is not null before calling tcp_sndbuf().

[dpgeorge]

This will change existing behaviour: a new socket is writable and a connecting socket can also be polled.

[rkojedzinszky]

I've compiled the two branches:
Compiling master branch:
('flash ', 35488)
('padding ', 1376)
('irom0text', 542108)
('total ', 578972)
('md5 ', 'b368e5b34b904155fb7bbaeb4286b89f')

Compiling with proposed fix:
('flash ', 35488)
('padding ', 1376)
('irom0text', 542124)
('total ', 578988)
('md5 ', '2b8357a9fdc04b1ca118294922d8caba')

That adds 16 bytes, but the patch is clear, will return expected results for sockets in NEW, CONNECTING or CONNECTED states. Other states (which are some kind of error states) will return the desired poll flag. The comments in handling the STATE_PEER_CLOSED block suggests the same principle. Actually, it does break existing code where the socket was in an error state, now an application will not block on it indefinitely.

Would you prefer a change like this?

diff --git a/extmod/modlwip.c b/extmod/modlwip.c
index bbb01b5d..e1b0ad10 100644
--- a/extmod/modlwip.c
+++ b/extmod/modlwip.c
@@ -1163,7 +1163,7 @@ STATIC mp_uint_t lwip_socket_ioctl(mp_obj_t self_in, mp_uint_t request, uintptr_
             ret |= MP_STREAM_POLL_RD;
         }
 
-        if (flags & MP_STREAM_POLL_WR && tcp_sndbuf(socket->pcb.tcp) > 0) {
+        if (flags & MP_STREAM_POLL_WR && (socket->pcb.tcp == NULL || tcp_sndbuf(socket->pcb.tcp) > 0)) {
             ret |= MP_STREAM_POLL_WR;
         }

While this would definetily be a smaller change in code, I still prefer the other. As that handles connection states, it is more clear about what id does. Also I dont know what other cases might exist when pcb.tcp is not null but the socket is closed in any way.

[dpgeorge]

In df078e8 I pushed a new test that tests for socket poll behaviour during a connect. This new test passes ok with CPython, unix uPy and the current esp8266 uPy code, so it describes the correct/desired behaviour of socket polling. But with this PR here part of the test is broken.

There are also libraries (like uasyncio) which depend critically on the behaviour of socket polling. The behaviour is defined by CPython and the unix port of uPy (which should match each other), and the modlwip.c code should for the most part already follow this behaviour. The issue you found with polling a closed socket should definitely be fixed but the existing behaviour for other polling cases shouldn't changed unless it is also wrong (ie doesn't follow CPython).

[rkojedzinszky]

Thanks for the comments, you are right, it really does not follow CPython. I will review, and propose another fix which will be in line with CPython.

[rkojedzinszky]

Unfortunately a simple test script does behave differently on Linux and on FreeBSD in different socket states. That way CPython itself is not consistent across platforms.

[rkojedzinszky]

Reading the code it seems that modlwip/poll method assumes the socket is a client tcp socket. No cases for udp or listening sockets. That should need a refactor to conform more to CPython / POSIX.