net: remove non-blocking bool from interface #22052

pull Bushstar wants to merge 1 commits into bitcoin:master from Bushstar:bitcoin-nonblocking-true changing 2 files +10 −22

Bushstar commented at 6:20 AM on May 25, 2021: contributor

SetSocketNonBlocking was added in 0.11 in the PR below with a second argument to toggle non-blocking mode for the socket. That argument has always been set to true in all subsequent releases and I'm not sure why it is present.

https://github.com/bitcoin/bitcoin/pull/4491
net: remove non-blocking bool from interface c71117fcb0
fanquake added the label P2P on May 25, 2021
MarcoFalke added the label Refactoring on May 25, 2021
MarcoFalke commented at 6:49 AM on May 25, 2021: member

Concept ACK. Looks like the last use of false was removed in commit 6050ab685553c7312ef105d2c4a5230c3fcf4002
DrahtBot commented at 7:42 AM on May 25, 2021: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #24357 (refactor: make setsockopt() and SetSocketNoDelay() mockable/testable by vasild)
- #21878 (Make all networking code mockable by vasild)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
promag commented at 11:17 AM on May 25, 2021: member

Code review ACK c71117fcb04fc2e45b5e76fe96b077a07b0c0f82.
laanwj commented at 12:57 PM on May 25, 2021: member

I'm not entirely sure this is an improvement. Even though sometimes some parts of an API are not used, there can be implicit expectations. E.g. say you have a vector type, you'd expect to have certain operations available in a data structure (add, insert, remove, iterate) even though they may not used for some time.

From the viewpoint of a sockets API it is, is the ability to switch blocking on and off on a socket the expectation? It somewhat seems like to me, that stripping this might be going a step to far. But no strong opinion either.
lsilva01 commented at 6:55 PM on May 29, 2021: contributor

Code review ACK https://github.com/bitcoin/bitcoin/pull/22052/commits/c71117fcb04fc2e45b5e76fe96b077a07b0c0f82

in src/netbase.cpp:726 in c71117fcb0

 727 | +    if (ioctlsocket(hSocket, FIONBIO, &nOne) == SOCKET_ERROR) {
 728 |  #else
 729 | -        int fFlags = fcntl(hSocket, F_GETFL, 0);
 730 | -        if (fcntl(hSocket, F_SETFL, fFlags | O_NONBLOCK) == SOCKET_ERROR) {
 731 | +    int fFlags = fcntl(hSocket, F_GETFL, 0);
 732 | +    if (fcntl(hSocket, F_SETFL, fFlags | O_NONBLOCK) == SOCKET_ERROR) {

vasild commented at 12:19 PM on July 2, 2021:

Out of the scope of this PR, feel free to ignore

Here we assume the first call to fcntl() always succeeds. If it returns -1 a chaos will ensue (the man does not say that it will always succeed with F_GETFL, maybe it could fail if interrupted by a signal).

vasild approved
vasild commented at 12:19 PM on July 2, 2021: member

ACK c71117fcb04fc2e45b5e76fe96b077a07b0c0f82

In #21878 in commit 4b60a7bf5 net: convert standalone SetSocketNonBlocking() to Sock::SetNonBlocking() that argument is also removed.

In general, I am in favor of removing unused code. If needed it can always be added or resurrected from git history.
fanquake commented at 1:57 PM on February 12, 2022: member

Concept ACK. Made this same change myself before realising there was already a PR open.
laanwj merged this on Apr 14, 2022
laanwj closed this on Apr 14, 2022
sidhujag referenced this in commit 4ecae9e1ad on Apr 14, 2022
DrahtBot locked this on Apr 14, 2023

Contributors

Labels

Linked (view graph)

#4491 net: add setSocketBlocking() as OS independent wrapper

net: remove non-blocking bool from interface #22052

Conflicts