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.

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

net: implement poll #14336

Merged
merged 5 commits into from Jan 2, 2019
Merged

Conversation

@pstratem
Copy link
Contributor

@pstratem pstratem commented Sep 26, 2018

Implement poll() on systems which support it properly.

This eliminates the restriction on maximum socket descriptor number.

@fanquake fanquake added the P2P label Sep 26, 2018
@pstratem pstratem force-pushed the 2018-09-24-socket-handler-poll branch 3 times, most recently from f5aa8cf to 6bcefaa Sep 27, 2018
@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Sep 27, 2018

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

Conflicts

No conflicts as of last run.

@pstratem pstratem force-pushed the 2018-09-24-socket-handler-poll branch from 6bcefaa to a97afdf Sep 27, 2018
@pstratem pstratem force-pushed the 2018-09-24-socket-handler-poll branch 5 times, most recently from 80e59ca to 7fd976b Sep 29, 2018
@pstratem
Copy link
Contributor Author

@pstratem pstratem commented Sep 29, 2018

It seems that lots of the functional tests have races cause this pull request keeps triggering random seeming failures.

src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@jimpo jimpo left a comment

Concept ACK

src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
int nErr = WSAGetLastError();
LogPrintf("socket select error %s\n", NetworkErrorString(nErr));
for (unsigned int i = 0; i <= hSocketMax; i++)
FD_SET(i, &fdsetRecv);
Copy link
Contributor

@jimpo jimpo Sep 29, 2018

I see this is the same as the code currently, but it seems like this should be fdsetError instead of fdsetRecv, even though they are handled exactly the same.

Copy link
Contributor Author

@pstratem pstratem Oct 1, 2018

I don't think it should be there at all really. select() failures dont mean the sockets themselves have failed, but i guess it could mean we made a mistake and called select() with a closed socket?

Copy link
Contributor

@jimpo jimpo Oct 2, 2018

Yeah, then iterating through each one and calling recv would identify which peer to disconnect. Though, if it's fdsetError and not fdsetRecv, it probably shouldn't disconnect when nBytes == 0.

@pstratem pstratem force-pushed the 2018-09-24-socket-handler-poll branch 3 times, most recently from 5cf67c9 to e4db5b3 Oct 1, 2018
@laanwj
Copy link
Member

@laanwj laanwj commented Oct 2, 2018

Concept ACK, will review in detail when my brain works again.

@pstratem pstratem force-pushed the 2018-09-24-socket-handler-poll branch from cd28216 to 499bdef Oct 2, 2018
@theuni
Copy link
Member

@theuni theuni commented Oct 2, 2018

Concept ACK. Will also review shortly.

@pstratem pstratem force-pushed the 2018-09-24-socket-handler-poll branch 2 times, most recently from 7acadee to 84fae8d Oct 16, 2018
Copy link
Member

@jamesob jamesob left a comment

Some minor comments and a question about conservatively falling back to select for MacOS systems. Code looks good!

src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
This separates the select() logic from the socket handling logic, setting up
for a switch to poll().
@pstratem pstratem force-pushed the 2018-09-24-socket-handler-poll branch from cc6edb4 to 20c9601 Nov 30, 2018
@pstratem
Copy link
Contributor Author

@pstratem pstratem commented Nov 30, 2018

With 87 connections the effect of generating the pollfd list shows up on a profile but isn't significant.

The poll() implementation is on the order of 1000 ns slower per call than the select() implementation.

I don't think that's significant.

jamesob
jamesob approved these changes Dec 3, 2018
Copy link
Member

@jamesob jamesob left a comment

utACK 20c9601

Code looks good, feel free to ignore the small comments. I'm running tests at the moment and will report back with results within the next day - testnet happy-path behavior looks good so far.

src/net.cpp Show resolved Hide resolved
}
}
#else
void CConnman::SocketEvents(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_set, std::set<SOCKET> &error_set)
Copy link
Member

@jamesob jamesob Dec 3, 2018

(non-blocking)

diff --git a/src/net.cpp b/src/net.cpp
index 7b8b6e5ea2..03d165d701 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1355,6 +1355,7 @@ void CConnman::SocketEvents(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_s
     }
 }
 #else
+// Use select() on platforms where poll() is unavailable.
 void CConnman::SocketEvents(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_set, std::set<SOCKET> &error_set)
 {
     std::set<SOCKET> recv_select_set, send_select_set, error_select_set;

src/netbase.cpp Outdated Show resolved Hide resolved
struct timeval tval = MillisToTimeval(std::min(endTime - curTime, maxWait));
fd_set fdset;
FD_ZERO(&fdset);
FD_SET(hSocket, &fdset);
int nRet = select(hSocket + 1, &fdset, nullptr, nullptr, &tval);
#endif
if (nRet == SOCKET_ERROR) {
Copy link
Member

@jamesob jamesob Dec 3, 2018

(non-blocking) This might be academic, but since this clause is handling both select() and poll() might be safer to say if (nRet < 0)? Though right now there's no difference in the return value for both APIs (and I doubt that'll ever change...).

@pstratem pstratem force-pushed the 2018-09-24-socket-handler-poll branch from 20c9601 to 4927bf2 Dec 3, 2018
jamesob
jamesob approved these changes Dec 4, 2018
Copy link
Member

@jamesob jamesob left a comment

Tested ACK 4927bf2

Tested on Linux 4.15.0-39-generic and macOS 10.12.1. Tested IBD on testnet as well as tweaking -maxconnections and observing connections with lsof -a -itcp -p $(pidof bitcoind).

Only code change since my last utACK is a small cleanup in netbase.cpp.

Thanks for the work here @pstratem.

@laanwj
Copy link
Member

@laanwj laanwj commented Dec 13, 2018

utACK 4927bf2

@laanwj laanwj merged commit 4927bf2 into bitcoin:master Jan 2, 2019
2 checks passed
laanwj added a commit that referenced this issue Jan 2, 2019
4927bf2 Increase maxconnections limit when using poll. (Patrick Strateman)
11cc491 Implement poll() on systems which support it properly. (Patrick Strateman)
28211a4 Move SocketEvents logic to private method. (Patrick Strateman)
7e403c0 Move GenerateSelectSet logic to private method. (Patrick Strateman)
1e6afd0 Introduce and use constant SELECT_TIMEOUT_MILLISECONDS. (Patrick Strateman)

Pull request description:

  Implement poll() on systems which support it properly.

  This eliminates the restriction on maximum socket descriptor number.

Tree-SHA512: b945cd9294afdafcce96d547f67679d5cdd684cf257904a239cd1248de3b5e093b8d6d28d8d1b7cc923dc0b2b5723faef9bc9bf118a9ce1bdcf357c2323f5573
@fanquake fanquake removed this from Blockers in High-priority for review Jan 3, 2019
lavie
Copy link

lavie commented on 11cc491 Jan 3, 2019

nit: broke -> broken

@BlockMechanic
Copy link
Contributor

@BlockMechanic BlockMechanic commented Oct 12, 2019

Crashes on android 7 8 and 9 armv7a. May need to implement a work around

codablock added a commit to codablock/dash that referenced this issue Apr 8, 2020
codablock added a commit to codablock/dash that referenced this issue Apr 8, 2020
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Jun 9, 2020
Summary:
Partial backport of Core [[bitcoin/bitcoin#14336 | PR14336]]
bitcoin/bitcoin@1e6afd0

Test Plan:
  ninja
  ninja check

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, majcosta, jasonbcox

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, majcosta, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D6459
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Jun 9, 2020
Summary:
This separates the socket event collection logic from the logic
deciding which events we're interested in at all.

Partial backport of Core [[bitcoin/bitcoin#14336 | PR14336]]
bitcoin/bitcoin@7e403c0

Depends on D6459

Test Plan:
  ninja
  ninja check-all
  src/bitcoind -debug=net
Verify normal node connection behavior

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien, jasonbcox

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien, jasonbcox

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D6460
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Jun 9, 2020
Summary:
This separates the select() logic from the socket handling logic, setting up
for a switch to poll().

Partial backport of Core [[bitcoin/bitcoin#14336 | PR14336]]
bitcoin/bitcoin@28211a4

Depends on D6460

Test Plan:
  ninja
  ninja check-all
  src/bitcoind -debug=net
Verify normal node behavior.

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, majcosta, jasonbcox

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, majcosta, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D6461
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Jun 9, 2020
Summary:
This eliminates the restriction on maximum socket descriptor number.

Partial backport of Core [[bitcoin/bitcoin#14336 | PR14336]]
bitcoin/bitcoin@11cc491

Depends on D6461

Test Plan:
  ninja
  ninja check-all
  src/bitoind
Verify normal node behavior.

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, majcosta

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D6470
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Jun 10, 2020
Summary:
Partial backport of Core [[bitcoin/bitcoin#14336 | PR14336]]
bitcoin/bitcoin@4927bf2

Depends on D6470

Test Plan:
  ninja
  ninja check-all
  src/bitcoind --debug=net
Verify normal node behavior.

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, majcosta

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, majcosta

Subscribers: majcosta

Differential Revision: https://reviews.bitcoinabc.org/D6471
hebasto added a commit to hebasto/bitcoin that referenced this issue Sep 25, 2021
laanwj added a commit that referenced this issue Sep 27, 2021
ee7891a doc: Remove outdated comments (Hennadii Stepanov)

Pull request description:

  The first removed comment was introduced in #5288, the second one in #13503.

  Both are outdated since #14336.

ACKs for top commit:
  duncandean:
    crACK ee7891a

Tree-SHA512: a2d6071919e81c916bfc2178109bbc464417321bcc567ed0644448c5faea8e58cb08a7657afa1b6ffe1fb63e114a2a47b31c893e471839ba9d49a3986e68b2a7
sidhujag added a commit to syscoin/syscoin that referenced this issue Sep 27, 2021
ee7891a doc: Remove outdated comments (Hennadii Stepanov)

Pull request description:

  The first removed comment was introduced in bitcoin#5288, the second one in bitcoin#13503.

  Both are outdated since bitcoin#14336.

ACKs for top commit:
  duncandean:
    crACK ee7891a

Tree-SHA512: a2d6071919e81c916bfc2178109bbc464417321bcc567ed0644448c5faea8e58cb08a7657afa1b6ffe1fb63e114a2a47b31c893e471839ba9d49a3986e68b2a7
laanwj added a commit that referenced this issue Sep 30, 2021
…t correctly

8ff3743 Revert "doc: Remove outdated comments" (Hennadii Stepanov)

Pull request description:

  Unfortunately, in #23094 the assumption that #14336 makes comments outdated is wrong. As pointed in  #23094 (comment), the #14336 just moved the relevant code a few lines down.

  This PR reverts commit ee7891a, and moves the comments into the right place.

  I apologize about that.

ACKs for top commit:
  MarcoFalke:
    cr ACK 8ff3743
  laanwj:
    ACK 8ff3743

Tree-SHA512: 84aca627bb5b49c06fc172778f9b9407482c5a873ccbc3dc40167e6a8ad0bc60475d6a469c843b7b42712e35cf3fc2d3518923e791d5e0c59628e042acc72747
sidhujag added a commit to syscoin/syscoin that referenced this issue Sep 30, 2021
… comment correctly

8ff3743 Revert "doc: Remove outdated comments" (Hennadii Stepanov)

Pull request description:

  Unfortunately, in bitcoin#23094 the assumption that bitcoin#14336 makes comments outdated is wrong. As pointed in  bitcoin#23094 (comment), the bitcoin#14336 just moved the relevant code a few lines down.

  This PR reverts commit ee7891a, and moves the comments into the right place.

  I apologize about that.

ACKs for top commit:
  MarcoFalke:
    cr ACK 8ff3743
  laanwj:
    ACK 8ff3743

Tree-SHA512: 84aca627bb5b49c06fc172778f9b9407482c5a873ccbc3dc40167e6a8ad0bc60475d6a469c843b7b42712e35cf3fc2d3518923e791d5e0c59628e042acc72747
rebroad pushed a commit to rebroad/bitcoin that referenced this issue Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet