fix windows polling using sockets for the WakeupPipe #5599

blagoev · 2022-06-16T22:09:40Z

RedBeard0531

I don't know how much effort we want to put into making this good great. Hopefully it will be dead soon(ish), but OTOH, maybe we should assume it will last longer than we expect...

RedBeard0531 · 2022-06-17T08:06:11Z

src/realm/util/network.cpp

+ do {
+ if (setsockopt(listener, SOL_SOCKET, SO_REUSEADDR, 
+ (char*) &reuse, (socklen_t) sizeof(reuse)) == -1)
+ break;
+ if (bind(listener, &a.addr, sizeof(a.inaddr)) == SOCKET_ERROR)
+ break;
+ if (getsockname(listener, &a.addr, &addrlen) == SOCKET_ERROR)
+ break;
+ if (listen(listener, 1) == SOCKET_ERROR)
+ break;
+ socks[0] = WSASocket(AF_INET, SOCK_STREAM, 0, NULL, 0, flags);
+ if (socks[0] == INVALID_SOCKET)
+ break;
+ if (connect(socks[0], &a.addr, sizeof(a.inaddr)) == SOCKET_ERROR)
+ break;
+ socks[1] = accept(listener, NULL, NULL);
+ if (socks[1] == INVALID_SOCKET)
+ break;
+
+ closesocket(listener);
+ return 0;
+
+ } while (0);


This do {} while(0)+break is basically just a disguised goto ERR;. Personally I think it would be better to just use a goto than hiding it in something that looks like a loop, but maybe others are more anti-goto than I am.

I agree its weird. I don't like using wholes for something other than a loop especially I don't like writing endless loops.
but I tried not to change anything on this code so we follow the already public version that I assume is tested and reviewed by others

src/realm/util/network.cpp

RedBeard0531 · 2022-06-17T09:19:17Z

src/realm/util/network.cpp

+ break;
+ if (connect(socks[0], &a.addr, sizeof(a.inaddr)) == SOCKET_ERROR)
+ break;
+ socks[1] = accept(listener, NULL, NULL);


There is a slight risk that some other process will connect to this port before we do. We could mitigate that by generating a secure random number and requiring the connecter to send that number and verify that it matches. But it seems like there is a ton of code (including the python stdlib) using some form of this pattern and none of it tries to handle this case, so it is probably overkill. Since this is a localhost listener, at least we don't have to worry about remote port scanners.

Edit: I guess if some other process did manage to beat us at connecting then we would probably have the connect call above either hang or return an error since we call listen with a backlog of 1. So maybe we would only get to this line if we were the one connecting.

RedBeard0531 · 2022-06-17T09:28:43Z

src/realm/util/network.cpp

+ socks[0] = WSASocket(AF_INET, SOCK_STREAM, 0, NULL, 0, flags);
+ if (socks[0] == INVALID_SOCKET)
+ break;
+ if (connect(socks[0], &a.addr, sizeof(a.inaddr)) == SOCKET_ERROR)


I'm somewhat surprised that this doesn't hang since we haven't set this to non-blocking. I noticed that the python impl sets it to nonblocking prior to connecting then sets it back to blocking. We could skip setting it back to blocking since I think we want it in non-blocking mode anyway.

Edit: This is related to the next comment. It looks like connect() only blocks until we enter the listener's backlog, since the connection is "pre-accepted" by the kernel for us prior to calling accept() as long as there is room in the backlog. Since we have a backlog of 1, we would only hang if some other process beat us to connecting. Maybe it is worth setting nonblocking so that we can cleanly error here rather than hanging indefinitely in this very unlikely case?

I guess this got reviewed a bunch of times from multiple people so I assume it is fine. Like this one https://git.infradead.org/users/dwmw2/openconnect.git/commitdiff/bdeefa54

I am hesitant to try to improve it since we risk introducing bugs which were not there.

RedBeard0531 · 2022-06-17T09:37:59Z

src/realm/util/network.cpp

+ m_write_fd.reset(socks[0]);
+ m_read_fd.reset(socks[1]);


Don't we need to set these to non-blocking? Or is that handled somewhere else?

I think these should be blocking, cause signal and acknowledge_signal should complete after the signal has been sent/read

src/realm/util/network.cpp

RedBeard0531 · 2022-06-17T09:55:14Z

src/realm/util/network.cpp

+ max_wait_millis);
+ /*if (ret == SOCKET_ERROR) {
+ int lastErr = WSAGetLastError();
+ }*/
 REALM_ASSERT(ret != SOCKET_ERROR);


Shouldn't we try to use the common error handling below (removing the #ifndef on line 1244)?

I am hesitant to do this much changes. So I left the error handling as it is on this platform.

RedBeard0531 · 2022-06-17T09:56:56Z

src/realm/util/network.cpp

 // Poll all network sockets
- ret = WSAPoll(LPWSAPOLLFD(&m_pollfd_slots[1]), ULONG(m_pollfd_slots.size() - 1),
- socket_poll_timeout);
+ ret = WSAPoll(LPWSAPOLLFD(fds), ULONG(m_pollfd_slots.size()),


I wonder if we could get rid of most of the ifdefs in this code by having a poll_wrapper function above that dispatches to WSAPoll on windows and ::poll everywhere else. At the very least, could we narrow the #ifdeffing to just be around the function call or is some of the other setup also platform specific?

I targeted of making as minimal changes as possible. I guess we could refactor the code more

src/realm/util/network.cpp

Co-authored-by: Kræn Hansen <kraen.hansen@mongodb.com> Co-authored-by: Kenneth Geisshirt <kenneth.geisshirt@mongodb.com> Co-authored-by: LJ <81748770+elle-j@users.noreply.github.com>

fix windows polling using sockets for the WakeupPipe

a996038

cla-bot bot added the cla: yes label Jun 16, 2022

blagoev mentioned this pull request Jun 16, 2022

Sleep while polling sockets on Windows #5594

Merged

3 tasks

blagoev requested review from RedBeard0531 and jbreams June 16, 2022 22:42

RedBeard0531 requested changes Jun 17, 2022

View reviewed changes

handle PR feedback

c98eb03

blagoev closed this Jun 17, 2022

blagoev deleted the blagoev/fix-win-polling branch June 17, 2022 17:52

blagoev restored the blagoev/fix-win-polling branch June 17, 2022 17:52

blagoev deleted the blagoev/fix-win-polling branch June 17, 2022 19:53

github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix windows polling using sockets for the WakeupPipe #5599

fix windows polling using sockets for the WakeupPipe #5599

blagoev commented Jun 16, 2022 •

edited

Loading

RedBeard0531 left a comment

RedBeard0531 Jun 17, 2022

blagoev Jun 17, 2022

RedBeard0531 Jun 17, 2022

RedBeard0531 Jun 17, 2022

blagoev Jun 17, 2022

RedBeard0531 Jun 17, 2022

blagoev Jun 17, 2022

RedBeard0531 Jun 17, 2022

blagoev Jun 17, 2022

RedBeard0531 Jun 17, 2022

blagoev Jun 17, 2022

fix windows polling using sockets for the WakeupPipe #5599

fix windows polling using sockets for the WakeupPipe #5599

Conversation

blagoev commented Jun 16, 2022 • edited Loading

RedBeard0531 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blagoev commented Jun 16, 2022 •

edited

Loading