Skip to content
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.

Sign up for GitHub

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

Jump to bottom

extmod/modlwip: Fixed socket poll in reset state #3450

Closed
wants to merge 1 commit into from
Closed

extmod/modlwip: Fixed socket poll in reset state #3450

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 15 additions & 9 deletions extmod/modlwip.c
View file
Open in desktop
Expand Up @@ -1159,19 +1159,25 @@ STATIC mp_uint_t lwip_socket_ioctl(mp_obj_t self_in, mp_uint_t request, uintptr_
uintptr_t flags = arg;
ret = 0;

if (flags & MP_STREAM_POLL_RD && socket->incoming.pbuf != NULL) {
ret |= MP_STREAM_POLL_RD;
}

if (flags & MP_STREAM_POLL_WR && tcp_sndbuf(socket->pcb.tcp) > 0) {
ret |= MP_STREAM_POLL_WR;
}
switch (socket->state) {
case STATE_NEW:
case STATE_CONNECTING:
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case STATE_CONNECTED:
if (flags & MP_STREAM_POLL_RD && socket->incoming.pbuf != NULL) {
ret |= MP_STREAM_POLL_RD;
}

if (socket->state == STATE_PEER_CLOSED) {
// Peer-closed socket is both readable and writable: read will
if (flags & MP_STREAM_POLL_WR && tcp_sndbuf(socket->pcb.tcp) > 0) {
ret |= MP_STREAM_POLL_WR;
}
break;
default:
// Any other state is both readable and writable: read will
// return EOF, write - error. Without this poll will hang on a
// socket which was closed by peer.
ret |= flags & (MP_STREAM_POLL_RD | MP_STREAM_POLL_WR);
break;
}

} else {
Expand Down