Make all networking code mockable #21878
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced May 7, 2021
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
This was referenced May 7, 2021
|
Strong Concept ACK: mockable means fuzzable. |
|
Concept ACK |
0256cfd to
e68bc63
Compare
|
|
|
Very nice. Strong concept ACK. |
e68bc63 to
5772edb
Compare
|
|
5772edb to
ec5f887
Compare
|
|
ec5f887 to
a0b0030
Compare
|
|
a0b0030 to
6bed261
Compare
|
|
Peeking at the underlying socket file descriptor of `Sock` and checkig if it is `INVALID_SOCKET` is bad encapsulation and stands in the way of testing/mocking/fuzzing. Instead use an empty unique_ptr to denote that there is no valid socket.
The socket is always valid (the underlying file descriptor is not `INVALID_SOCKET`) when `GetBindAddress()` is called.
The socket is always valid (the underlying file descriptor is not `INVALID_SOCKET`) when `ConnectSocketDirectly()` is called.
`Sock::Get()` was used only in `sock.{cpp,h}`. Remove it and access
`Sock::m_socket` directly.
Unit tests that used `Get()` to test for equality still verify that the
behavior is correct, indirectly, by testing whether the socket is closed
or not.
Now that all network calls done by `CConnman::OpenNetworkConnection()` are done via `Sock` they can be redirected (mocked) to `FuzzedSocket` for testing.
b497200 to
80d9dfd
Compare
|
|
This was referenced Aug 17, 2023
ryanofsky
added a commit
that referenced
this pull request
Oct 3, 2023
7df4508 test: improve sock_tests/move_assignment (Vasil Dimov) 5086a99 net: remove Sock default constructor, it's not necessary (Vasil Dimov) 7829272 net: remove now unnecessary Sock::Get() (Vasil Dimov) 944b21b net: don't check if the socket is valid in ConnectSocketDirectly() (Vasil Dimov) aeac68d net: don't check if the socket is valid in GetBindAddress() (Vasil Dimov) 5ac1a51 i2p: avoid using Sock::Get() for checking for a valid socket (Vasil Dimov) Pull request description: _This is a piece of #21878, chopped off to ease review._ Peeking at the underlying socket file descriptor of `Sock` and checkig if it is `INVALID_SOCKET` is bad encapsulation and stands in the way of testing/mocking/fuzzing. Instead use an empty `unique_ptr` to denote that there is no valid socket where appropriate or outright remove such checks where they are not necessary. The default constructor `Sock::Sock()` is unnecessary now after recent changes, thus remove it. ACKs for top commit: ajtowns: ACK 7df4508 jonatack: ACK 7df4508 Tree-SHA512: 9742aeeeabe8690530bf74caa6ba296787028c52f4a3342afd193b05dbbb1f6645935c33ba0a5230199a09af01c666bd3c7fb16b48692a0d185356ea59a8ddbf
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
All the functional bits of this are now merged. Thank you all! This enables extending the test framework to be able to call any method, without worries that it may end up doing OS syscalls (try to create sockets, open connections, etc). A PR that does some of that is in #28584 (= last 4 commits from this PR). Closing as complete. |
Frank-GER
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 13, 2023
7df4508 test: improve sock_tests/move_assignment (Vasil Dimov) 5086a99 net: remove Sock default constructor, it's not necessary (Vasil Dimov) 7829272 net: remove now unnecessary Sock::Get() (Vasil Dimov) 944b21b net: don't check if the socket is valid in ConnectSocketDirectly() (Vasil Dimov) aeac68d net: don't check if the socket is valid in GetBindAddress() (Vasil Dimov) 5ac1a51 i2p: avoid using Sock::Get() for checking for a valid socket (Vasil Dimov) Pull request description: _This is a piece of bitcoin#21878, chopped off to ease review._ Peeking at the underlying socket file descriptor of `Sock` and checkig if it is `INVALID_SOCKET` is bad encapsulation and stands in the way of testing/mocking/fuzzing. Instead use an empty `unique_ptr` to denote that there is no valid socket where appropriate or outright remove such checks where they are not necessary. The default constructor `Sock::Sock()` is unnecessary now after recent changes, thus remove it. ACKs for top commit: ajtowns: ACK 7df4508 jonatack: ACK 7df4508 Tree-SHA512: 9742aeeeabe8690530bf74caa6ba296787028c52f4a3342afd193b05dbbb1f6645935c33ba0a5230199a09af01c666bd3c7fb16b48692a0d185356ea59a8ddbf
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Feb 27, 2024
a724c39 net: rename Sock::Reset() to Sock::Close() and make it private (Vasil Dimov) e8ff3f0 net: remove CloseSocket() (Vasil Dimov) 175fb26 net: remove now unused Sock::Release() (Vasil Dimov) Pull request description: _This is a piece of bitcoin#21878, chopped off to ease review._ * `Sock::Release()` is unused, thus remove it * `CloseSocket()` is only called from `Sock::Reset()`, so move the body of `CloseSocket()` inside `Sock::Reset()` and remove `CloseSocket()` - this helps to hide low level file descriptor sockets inside the `Sock` class. * Rename `Sock::Reset()` to `Sock::Close()` and make it `private` - to be used only in the destructor and in the `Sock` assignment operator. This simplifies the public API by removing one method from it. ACKs for top commit: laanwj: Code review ACK a724c39 Tree-SHA512: 4b12586642b3d049092fadcb1877132e285ec66a80af92563a7703c6970e278e0f2064fba45c7eaa78eb65db94b3641fd5e5264f7b4f61116d1a6f3333868639
5 tasks
PastaPastaPasta
added a commit
to dashpay/dash
that referenced
this pull request
May 14, 2024
…keup pipes logic out of `CConnman` and into `EdgeTriggeredEvents` and `WakeupPipes` bd8b5d4 net: add more details to log information in ETE and `WakeupPipes` (Kittywhiskers Van Gogh) ec99294 net: restrict access `EdgeTriggerEvents` members (Kittywhiskers Van Gogh) f24520a net: log `close` failures in `EdgeTriggerEvents` and `WakeupPipe` (Kittywhiskers Van Gogh) b8c3b48 refactor: introduce `WakeupPipe`, move wakeup select pipe logic there (Kittywhiskers Van Gogh) ed7d976 refactor: move wakeup pipe (de)registration to ETE (Kittywhiskers Van Gogh) f50c710 refactor: move `CConnman::`(`Un`)`registerEvents` to ETE (Kittywhiskers Van Gogh) 3a9f386 refactor: move `SOCKET` addition/removal from interest list to ETE (Kittywhiskers Van Gogh) 212df06 refactor: introduce `EdgeTriggeredEvents`, move {epoll, kqueue} fd there (Kittywhiskers Van Gogh) 3b11ef9 refactor: move `CConnman::SocketEventsMode` to `util/sock.h` (Kittywhiskers Van Gogh) Pull request description: ## Motivation `CConnman` is an entity that contains a lot of platform-specific implementation logic, both inherited from upstream and added upon by Dash (support for edge-triggered socket events modes like `epoll` on Linux and `kqueue` on FreeBSD/Darwin). Bitcoin has since moved to strip down `CConnman` by moving peer-related logic to the `Peer` struct in `net_processing` (portions of which are backported in #5982 and friends, tracking efforts from bitcoin#19398) and moving socket-related logic to `Sock` (portions of which are aimed to be backported in #6004, tracking efforts from bitcoin#21878). Due to the direction being taken and the difference in how edge-triggered events modes operate (utilizing interest lists and events instead of iterating over each socket) in comparison to level-triggered modes (which are inherited from upstream), it would be reasonable to therefore, isolate Dash-specific code into its own entities and minimize the information `CConnman` has about its internal workings. One of the visible benefits of this approach is comparing `develop` (as of this writing, d44b0d5) and this pull request for interactions between wakeup pipes logic and {`epoll`, `kqueue`} logic. This is what construction looks like: https://github.com/dashpay/dash/blob/d44b0d5dcb9b54821d582b267a8b92264be2da1b/src/net.cpp#L3358-L3397 But, if we segment wakeup pipes logic (that work on any platform with POSIX APIs and excludes Windows) and {`epoll`, `kqueue`} logic (calling them `EdgeTriggeredEvents` instead), construction looks different: https://github.com/dashpay/dash/blob/907a3515170abed4ce9018115ed591e6ca9f4800/src/util/wpipe.cpp#L12-L38 Now wakeup pipes logic doesn't need to know what socket events mode is being used nor are the implementation aspects of (de)registering it its concern, that is now `EdgeTriggeredEvents` problem. ## Additional Information * This pull request will need testing on macOS (FreeBSD isn't a tier-one target) to ensure that lack of breakage in `kqueue`-specific logic. ## Breaking Changes * Dependency for #6018 * More logging has been introduced and existing log messages have been made more exhaustive. If there is parsing that relies on a particular template, they will have to be updated. * If `EdgeTriggeredEvents` or `WakeupPipes` fail to initialize or are incorrectly initialized and not destroyed immediately, any further attempts at calling any of its functions will result in an `assert`-induced crash. Earlier behavior may have allowed for silent failure but segmentation of logic from `CConnman` means the newly created instances must only exist if the circumstances needed for it to initialize correctly are present. This is to ensure that `CConnman` doesn't have to concern itself with internal workings of either entities. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK bd8b5d4 Tree-SHA512: 8f793d4b4f2d8091e05bb9cc108013e924bbfbf19081290d9c0dfd91b0f2c80652ccf853f1596562942b4433509149c526e111396937988db605707ae1fe7366
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a roadmap PR. It can be merged, but it can also be split into separate PRs and to get proper thorough review it is split.
Add wrapper methods to the syscalls
accept(),setsockopt(),getsockname(),bind(),listen()in theSockclass (e.g.Sock::Accept()). Those methods can be overriden (mocked) by unit tests (existent example inmaster) and by fuzz tests (existent example inmaster).Change everybody to use
Sockinstead ofSOCKET.Move the functionality of
CConnman::SocketEvents()to a mockable method of theSockclass.Already merged pieces of this PR
Current pieces of this for review