refactor: replace CConnman::SocketEvents() with mockable Sock::WaitMany() #24356
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
Approach ACK, first pass tested almost-ACK af45eee. Read the code, each commit debug builds cleanly / unit tests green, ran signet bitcoind on this branch rebased to master. As well as improving testability, this seems like a nice clean-up too. Reviewers may find man select helpful.
|
|
There was a problem hiding this comment.
ACK c9420b3 per re-review of git diff af45eee c9420b3 following my previous #24356 (review), rebased to master at 05957a8, debug build with Debian clang version 15, unit tests
|
Invalidates ACK from @jonatack |
src/net.h
Outdated
| EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex); | ||
|
|
||
| /** | ||
| * Accept incoming connections, one from each read-ready listening socket. | ||
| * @param[in] recv_set Sockets that are ready for read. | ||
| * @param[in] what Sockets that are ready for IO. |
There was a problem hiding this comment.
If you agree, maybe change the names of the what method params to ones that are more descriptive and unique (e.g. socket_data, wait_sockets, wait_data, sockets_ready_for_io, etc.) and for the what localvars, either the same or i.e. s where unimportant.
There was a problem hiding this comment.
Renamed to EventsPerSock. That map contains, per socket, the requested and the occurred events (just like poll(2)). And the variables then become the blatant events_per_sock ;-)
There was a problem hiding this comment.
Thanks, that seems better. In the last commit, I sort of hesitated in the two places where you do @param[in] events_per_sock Sockets that are ready for IO. ... thinking it should be something like "Events for each socket that is ready for IO."
|
Invalidates ACK from @jonatack |
|
re-ACK 6747729 |
|
Anyone else like to have a look here? |
|
Yes, I'll have a look. |
This mimics closely `CConnman::SocketEvents()` and the underlying `poll(2)`.
It allows waiting concurrently on more than one socket. Being a `virtual` `Sock` method it can be overriden by tests. Will be used to replace `CConnman::SocketEvents()`.
|
Invalidates ACK from @jonatack |
Rename `GenerateSelectSet()` to `GenerateWaitSockets()` and adapt it to generate a wait data suitable for `Sock::WaitMany()`. Then call it from `CConnman::SocketHandler()` and feed the generated data to `Sock::WaitMany()`. This way `CConnman::SocketHandler()` can be unit tested because `Sock::WaitMany()` is mockable, so the usage of real sockets can be avoided. Resolves bitcoin#21744
|
Previously invalidated ACK from @jonatack. |
|
There's a fuzz addressSAN issue, but it seems unrelated and there is the same one on the last push on master https://cirrus-ci.com/task/4544256453902336. |
|
Code review ACK 6e68ccb |
…mockable Sock::WaitMany() 6e68ccb net: use Sock::WaitMany() instead of CConnman::SocketEvents() (Vasil Dimov) ae26346 net: introduce Sock::WaitMany() (Vasil Dimov) cc74459 net: also wait for exceptional events in Sock::Wait() (Vasil Dimov) Pull request description: _This is a piece of bitcoin#21878, chopped off to ease review._ `Sock::Wait()` waits for IO events on one socket. Introduce a similar `virtual` method `WaitMany()` that waits simultaneously for IO events on more than one socket. Use `WaitMany()` instead of `CConnman::SocketEvents()` (and ditch the latter). Given that the former is a `virtual` method, it can be mocked by unit and fuzz tests. This will help to make bigger parts of `CConnman` testable (unit and fuzz). ACKs for top commit: laanwj: Code review ACK 6e68ccb jonatack: re-ACK 6e68ccb per `git range-diff e18fd47 6747729 6e68ccb`, and verified rebase to master and debug build Tree-SHA512: 917fb6ad880d64d3af1ebb301c06fbd01afd8ff043f49e4055a088ebed6affb7ffe1dcf59292d822f10de5f323b6d52d557cb081dd7434634995f9148efcf08f
As far as I can tell, the code calling for these includes was removed in: 6e68ccb bitcoin#24356 82d360b bitcoin#21387
This is a piece of #21878, chopped off to ease review.
Sock::Wait()waits for IO events on one socket. Introduce a similarvirtualmethodWaitMany()that waits simultaneously for IO events on more than one socket.Use
WaitMany()instead ofCConnman::SocketEvents()(and ditch the latter). Given that the former is avirtualmethod, it can be mocked by unit and fuzz tests. This will help to make bigger parts ofCConnmantestable (unit and fuzz).