This is a piece of #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).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
143 * @param[out] occurred If not nullptr and `true` is returned, then upon return this
144- * indicates which of the requested events occurred. A timeout is indicated by return
145- * value of `true` and `occurred` being set to 0.
146+ * indicates which of the requested events occurred (`ERR` will be added, even if
147+ * not requested, if an exceptional event occurs on the socket).
148+ * A timeout is indicated by return value of `true` and `occurred` being set to 0.
c1349291159b97de03160dfe4cbd9be7053b9dba maybe, if I understand correctly:
0 * Wait for readiness for input (recv) or output (send).
1 * [@param](/bitcoin-bitcoin/contributor/param/)[in] timeout Wait this much for at least one of the requested events to occur.
2 * [@param](/bitcoin-bitcoin/contributor/param/)[in] requested Wait for those events, bitwise-or of `RECV` and `SEND`.
3- * [@param](/bitcoin-bitcoin/contributor/param/)[out] occurred If not nullptr and `true` is returned, then upon return this
4+ * [@param](/bitcoin-bitcoin/contributor/param/)[out] occurred If not nullptr and the function returns `true`, then this
5 * indicates which of the requested events occurred (`ERR` will be added, even if
6 * not requested, if an exceptional event occurs on the socket).
7- * A timeout is indicated by return value of `true` and `occurred` being set to 0.
8- * [@return](/bitcoin-bitcoin/contributor/return/) true on success and false otherwise
9+ * [@return](/bitcoin-bitcoin/contributor/return/) true on success (or timeout, if `occurred` of 0 is returned), false otherwise
10 */
198+ * @param[in,out] what Wait for the requested events on these sockets and set `occurred`
199+ * to the events that actually occur.
200+ * A timeout is indicated by return value of `true` and all `what[].occurred`
201+ * being set to 0.
202+ * @return true on success and false otherwise
203+ */
fc4bfdf maybe, if I understand correctly:
0- * to the events that actually occur.
1- * A timeout is indicated by return value of `true` and all `what[].occurred`
2- * being set to 0.
3- * [@return](/bitcoin-bitcoin/contributor/return/) true on success and false otherwise
4+ * for the events that actually occurred.
5+ * [@return](/bitcoin-bitcoin/contributor/return/) true on success (or timeout, if all `what[].occurred` are returned as 0), false otherwise
6 */
192+ */
193+ using WaitData = std::unordered_map<std::shared_ptr<const Sock>, Events, Hash, Equal>;
194+
195+ /**
196+ * Same as `Wait()`, but wait on many sockets within the same timeout.
197+ * @param[in] timeout Wait this much for at least one of the requested events to occur.
183+ };
184+
185+ /**
186+ * On which socket to wait for what events in `WaitMany()`.
187+ * The `shared_ptr` is copied into the map to ensure that the `Sock` object
188+ * is not destroyed and the underlying socket closed. If this happens
1024 * Accept incoming connections, one from each read-ready listening socket.
1025- * @param[in] recv_set Sockets that are ready for read.
1026+ * @param[in] what Sockets that are ready for IO.
1027 */
1028- void SocketHandlerListening(const std::set<SOCKET>& recv_set);
1029+ void SocketHandlerListening(Sock::WaitData& what);
af45eee what is an in-param
0 void SocketHandlerListening(const Sock::WaitData& what);
1536@@ -1659,13 +1537,14 @@ void CConnman::SocketHandlerConnected(const std::vector<CNode*>& nodes,
1537 }
1538 }
1539
1540-void CConnman::SocketHandlerListening(const std::set<SOCKET>& recv_set)
1541+void CConnman::SocketHandlerListening(Sock::WaitData& what)
af45eee what is an in-param
0void CConnman::SocketHandlerListening(const Sock::WaitData& what);
1016 */
1017- void SocketHandlerConnected(const std::vector<CNode*>& nodes,
1018- const std::set<SOCKET>& recv_set,
1019- const std::set<SOCKET>& send_set,
1020- const std::set<SOCKET>& error_set);
1021+ void SocketHandlerConnected(const std::vector<CNode*>& nodes, Sock::WaitData& what);
af45eee what is an in-param
0 void SocketHandlerConnected(const std::vector<CNode*>& nodes, const Sock::WaitData& what);
1583
1584-void CConnman::SocketHandlerConnected(const std::vector<CNode*>& nodes,
1585- const std::set<SOCKET>& recv_set,
1586- const std::set<SOCKET>& send_set,
1587- const std::set<SOCKET>& error_set)
1588+void CConnman::SocketHandlerConnected(const std::vector<CNode*>& nodes, Sock::WaitData& what)
af45eee what is an in-param
0void CConnman::SocketHandlerConnected(const std::vector<CNode*>& nodes, const Sock::WaitData& what)
man select helpful.
af45eeea03...c9420b3cce: address suggestions
1010 EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
1011
1012 /**
1013 * Accept incoming connections, one from each read-ready listening socket.
1014- * @param[in] recv_set Sockets that are ready for read.
1015+ * @param[in] what Sockets that are ready for IO.
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.
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 ;-)
@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.”
git range-diff 132d5f8 c9420b3 ca8dcfa, rebase-only for added thread safety annotations and assertions
149@@ -150,7 +150,7 @@ bool Session::Accept(Connection& conn)
150 throw std::runtime_error("wait on socket failed");
151 }
152
153- if ((occurred & Sock::RECV) == 0) {
154+ if (occurred == 0) {
155 // Timeout, no incoming connections within MAX_WAIT_FOR_IO.
1001- */
1002- void SocketEvents(const std::vector<CNode*>& nodes,
1003- std::set<SOCKET>& recv_set,
1004- std::set<SOCKET>& send_set,
1005- std::set<SOCKET>& error_set);
1006+ Sock::EventsPerSock GenerateWaitSockets(const std::vector<CNode*>& nodes);
Instead of const std::vector<CNode*>& could use a std::span<const CNode*> which is more general?
Edit: although this is symmetrical with CConnman::GenerateWaitSockets.
Leaving it as is - I tried to make it Span, but it can’t be to const CNode* because the method does LOCK(pnode->cs_vSend); for each CNode element. Span<CNode*> argument results in the error: No viable conversion from 'const std::vector<CNode *>' to 'Span<CNode *>'.
It is symmetrical with CConnman::SocketHandlerConnected().
std::span<const CNode*> to be convertable from const std::vector<CNode *> Otherwise it’s a span of modifiable CNode pointers?
Alright, having it Span<CNode* const> works (not Span<const CNode*>)! The pointer itself has to be constant - should not be able to re-point it to another CNode object. And that pointer is pointing to a non-constant CNode object, so that we can modify it (or at least lock its member mutex which is equivalent to modifying). Changed!
PS std::span is a C++20 thing, we are using our own Span while on C++17.
Great!
PS std::span is a C++20 thing, we are using our own Span while on C++17.
Eh, yes, true.
210+ * @param[in,out] events_per_sock Wait for the requested events on these sockets and set
211+ * `occurred` for the events that actually occurred.
212+ * @return true on success (or timeout, if all `what[].occurred` are returned as 0),
213+ * false otherwise
214+ */
215+ [[nodiscard]] virtual bool WaitMany(std::chrono::milliseconds timeout,
Maybe return std::optional<EventsPerSock> instead of using output parameter.
Edit: never mind, it’s good like this, as it’s an in+out parameter.
172+ Event requested;
173+ Event occurred;
174+ };
175+
176+ struct Hash {
177+ size_t operator()(const std::shared_ptr<const Sock>& s) const
As this code is templated in the header anyway, could we do something more general than hardcoding std::shared_ptr<const Sock> here? I mean, it would work the same for any kind of “pointer to Sock”, whether shared, unique, or raw. Same for Equal below.
Edit: OK, so looking further, Hash and Equal are implementation details internal to the EventsPerSock structure. Another option would be to name them less generally.
shared_ptr because it is used for std::unordered_map with keys that are shared_ptr. Renamed to HashSharedPtrSock and EqualSharedPtrSock, better now?
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()`.
1588 // listening sockets in one call ("readiness" as in poll(2) or
1589 // select(2)). If none are ready, wait for a short while and return
1590 // empty sets.
1591- SocketEvents(snap.Nodes(), recv_set, send_set, error_set);
1592+ events_per_sock = GenerateWaitSockets(snap.Nodes());
1593+ if (events_per_sock.empty() || !events_per_sock.begin()->first->WaitMany(timeout, events_per_sock)) {
Sock::WaitMany be an instance method instead of a static one? It acts on multiple sockets at once, not one socket, after all, does it matter which one you call it on?
This looks slightly circuitous to me.
It is!
Is there a reason to have Sock::WaitMany be an instance method instead of a static one?
Yes. It is easier to override/mock it this way in tests.
It acts on multiple sockets at once, not one socket, after all, does it matter which one you call it on?
No it does not matter. Indeed it does not access any of the member variables and acts only on the sockets that are passed as arguments. As such it is more natural for it to be a standalone function or a static method of the Sock class.
I chose to have it as virtual method of the Sock class because this way it is easier for tests to provide a mocked alternative of it - just by providing a mocked Sock class, e.g. FuzzedSock.
If WaitMany() is a standalone function it would have to be done in a similar way like CreateSock() and g_dns_lookup() - global variables that point to “real” functions but are re-pointed to mocked functions in tests. So, each test that wants to mock those would have to override WaitMany() in addition to CreateSock(). Should I try to do it like that?
EventManager or such) that this could be a method on, and abusing a global function would be quite ugly too.
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 https://github.com/bitcoin/bitcoin/issues/21744
git range-diff e18fd47 6747729 6e68ccb, and verified rebase to master and debug build