net: Refactor ThreadSocketHandler #14147

This assignment is redundant, right? :-)

It is indeed.

Commit "Improve lock handling in DisconnectNodes."

Is it worth preventing notifying the same connection count?

Yes, it triggers a notification that updates the GUI and would happen every 50ms.

Commit "Improve lock handling in DisconnectNodes."

nit, space after if.

fixed

Commit "Move InactivityChecks logic to private method."

Just noting that with this change InactivityCheck is now called with cs_vNodes locked.

The reasoning here is that the contents of InactivityCheck are virtually instant.

If this causes any issues in the future it can be trivially switched back to using reference counting.

Commit "Merge InactivityCheck into InactivityChecks."

Could remove this block?

Not sure what you mean here

I mean like this:

void CConnman::InactivityChecks() {
    LOCK(cs_vNodes);
    int64_t nMicroTime = GetTimeMicros();
    int64_t nTime = nMicroTime / 1000000;
    for (CNode* pnode : vNodes) {
        if (nTime - pnode->nTimeConnected > 60)
        ...

yeah good point, the lock could take a while

Commit "Merge InactivityCheck into InactivityChecks."

Should be after LOCK(cs_vNodes)?

Not sure what you mean here

Move the call GetTimeMicros after the lock?

Ah I see

in commit e92a917117 Reduce calls to GetMicroTime in InactivityCheck.

newly introduced variables should always be snake_case, e.g. time_micro = ... .

In commit "Move DisconnectNodes logic to private method." (7c203fb5ab2558cd6e306d85a62c8bbe159a7c02)

Should delete this comment I think. It just takes up space and conveys no information.

In commit "Move DisconnectNodes logic to private method." (7c203fb5ab2558cd6e306d85a62c8bbe159a7c02)

Opening brace should go on next line.

In commit "Improve lock handling in DisconnectNodes." (57bd308a09556960e8fab1cd44f61ef519058a59)

I would combine this commit with the previous commit "Remove nPrevNodeCount parameter" because changes in that commit like moving the vNodesSize declaration further away from its assignment, and updating the nPrevNodeCount value before throwing it away really don't make sense standalone.

In commit "Move DisconnectNodes logic to private method." (2d5c345507208d215c0621180305a9f1d363012a)

It seems like moving NotifyNumConnectionsChanged call from right after the disconnect to later in the loop below after the select call changes behavior. So instead of ui being updated immediately after a disconnection, it will only be updated after a disconnect followed by a receive on another socket? Is this correct / intentional / good?

Select timeout is 50ms, doesn't seem to particularly matter.

Can certainly change it back though, it really shouldn't matter.

In commit "Move NotifyNumConnectionsChanged logic to private method." (e817ce3090fa71c61170216090685ee142d0f1d5)

It would be nice if you combined this commit with commit "Move DisconnectNodes logic to private method" (2d5c345507208d215c0621180305a9f1d363012a), or moved this commit before it, so this block of code only moves once instead of twice inside the same PR.

I've restructured the first commit so that the logic isn't moved the first time.

In commit "Run InactivityChecks before other socket handling logic." (8491dfbadceb8052f852ebf0020a622194db525f)

It seems like this commit should have no effect, but if it has no effect, then it's not clear why it would be desirable? I think it would be helpful if the commit messages in this PR said explicitly whether the commit is or isn't intending to change behavior.

It doesn't really matter, but running it earlier might allow for marking nodes disconnected before the socket handling logic runs.