Use Sock in CNode #23604
Use Sock in CNode #23604
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. |
vasild
force-pushed
the
Sock_in_CNode
branch
from
ba8cc5b
to
846e995
Compare
|
Invalidates (light) ACK from @theStack
Oh, good catch! The idea is to use |
|
Previously invalidated (light) ACK from @theStack |
|
Invalidates (light) ACK from @theStack |
|
Concept ACK. CI failures are unrelated. |
|
ACK d22a477 |
This way it can be used in `ConsumeNode()`.
|
Invalidates ACKs from @jonatack, @theStack, @stratospher, @w0xlt |
| break; | ||
| nBytes = send(node.hSocket, reinterpret_cast<const char*>(data.data()) + node.nSendOffset, data.size() - node.nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT); | ||
| } | ||
| nBytes = node.m_sock->Send(reinterpret_cast<const char*>(data.data()) + node.nSendOffset, data.size() - node.nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT); |
There was a problem hiding this comment.
Probably a silly question: LOCK(node.m_sock_mutex); applies even to node.m_sock->Send(...) operation but then here one can return contained sockets out of the function.
I understand that m_sock_mutex guards only m_sock member read & write operations but not necessarily the contained socket. Is that correct or not?
There was a problem hiding this comment.
That is a good question.
Before this PR the mutex protects the value of the socket variable - that is, to make sure it does not change from e.g. 7 (it is a file descriptor) to INVALID_SOCKET in the middle of constructs like:
if (socket != INVALID_SOCKET) {
... do something with the socket, assuming it is valid ...
}After this PR, it is the same with the exception that the variable is not a bare integer anymore, but a shared_ptr and an empty such pointer is used to designate what was designated before by INVALID_SOCKET.
Change `CNode` to use a pointer to `Sock` instead of a bare `SOCKET`. This will help mocking / testing / fuzzing more code.
-BEGIN VERIFY SCRIPT- sed -i -e 's/cs_hSocket/m_sock_mutex/g' $(git grep -l cs_hSocket) -END VERIFY SCRIPT-
|
Invalidates ACK from @jonatack Previously invalidated ACKs from @theStack, @stratospher, @w0xlt |
|
re-ACK ef5014d changes since last review are the removal of an unneeded dtor and the addition of a style commit |
| @@ -2994,11 +2997,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const | |||
| m_serializer = std::make_unique<V1TransportSerializer>(V1TransportSerializer()); | |||
| } | |||
|
|
|||
| CNode::~CNode() | |||
There was a problem hiding this comment.
Apologies if this is a silly question but just wanted to clarify - removing this destructor, it's implicit that the shared_ptr for Sock will be reset and the Sock's destructor will responsible for closing the socket?
There was a problem hiding this comment.
See https://en.cppreference.com/w/cpp/language/destructor "Implicitly-declared destructor".
The default destructor for CNode will call all the destructors of all of it's members. The destructor for shared_ptr will reduce the reference counter, and then if that reference counter is zero, the destructor for Sock will be called. If the RC is not zero, then the Sock will continue to exist on the heap (presumably being used by another thread), until the owner of that shared_ptr calls the destructor.
There was a problem hiding this comment.
utACK ef5014d, I have reviewed the code, and believe it makes sense to merge
This is a piece of #21878, chopped off to ease review.
Change
CNodeto use a pointer toSockinstead of a bareSOCKET.This will help mocking / testing / fuzzing more code.