Port NetCli/NetGameLib to standard C++ containers #1487
Conversation
The code in the Connect functions already ensured that the lists never contain multiple connections, so there's no need to use lists at all.
| static CliAuConn* s_conn = nullptr; | ||
| static CliAuConn * s_active; |
There was a problem hiding this comment.
Are there ever any times that s_conn and s_active will not be the same? I'm wondering if we could just use s_active instead...
There was a problem hiding this comment.
That's what I wanted to do initially, but unfortunately the two aren't exactly the same. s_conns/s_conn is always set before AsyncSocketConnect, whereas s_active is only set later once the connection is fully established (e. g. for the auth server only after a Auth2Cli_ClientRegisterReply is received).
This logic can probably be simplified somehow so that it only uses one variable, but I didn't want to do anything too risky as part of the legacy data structure cleanup.
There was a problem hiding this comment.
That seems like a good approach. 👍
| @@ -1327,8 +1326,7 @@ static CliAuConn * GetConnIncRef (const char tag[]) { | |||
| } | |||
|
|
|||
| //============================================================================ | |||
| static void UnlinkAndAbandonConn_CS (CliAuConn * conn) { | |||
| s_conns.Unlink(conn); | |||
| static void AbandonConn(CliAuConn* conn) { | |||
There was a problem hiding this comment.
I think this is my last question, but, just to verify... Does the caller not need to have the critical section locked anymore? I'm assuming no because this is no longer accessing any static variables, and you dropped _CS from the function name. If yes, we probably want to retain the _CS suffix on the function name.
There was a problem hiding this comment.
Exactly. As far as I can tell, the s_critsect lock was only needed to safely update s_conns, so now that this function doesn't do that anymore, it should be safe to call without the s_critsect locked (although currently all calls still have the lock).
That said, now that I'm re-reading this... maybe AbandonConn should lock on conn->critsect instead, because it manipulates a few fields directly instead of just calling methods? Honestly, I'm not quite sure which parts need which lock exactly (and I think the existing code is a bit inconsistent too).
There was a problem hiding this comment.
It's all a little muddy, for sure, so I took a look at the code. It looks like CliAuConn's ping timer reconnect stuff what requires the lock on CliAuConn::critsect, so I don't think we need to lock that here. OTOH, the code does seem to lock s_critsect when accessing or mutating any CliAuConn instance's cancelId field (see NotifyConnSocketConnectFailed and the other notification callbacks), so I think we still need to lock s_critsect before calling this function and rename it to AbandonConn_CS. I'm assuming the other CliConn variants have the same ownership model :/
Edit: Alternatively, you could just lock s_critsect inside AbandonConn and remove all of the manual locking that took place before calling AbandonConn_CS. The lock-then-call antipattern looks like a side effect of Cyan not having a recursive mutex. All of the "critical sections" in question here are just recursive mutexes now.
There was a problem hiding this comment.
Alternatively, you could just lock
s_critsectinsideAbandonConn
That seems like the best solution, yeah.
Not completely sure if this is actually necessary, but all other code that uses the abandoned and cancelId fields also holds s_critsect. Currently, all callers of AbandonConn already have s_critsect locked, but this is clearer and safer in case the calling code changes. Co-authored-by: Adam Johnson <[email protected]>
This eliminates the last uses of the weird custom intrusive linked list pnUtList (aka
LISTDECL/LIST/LINK).Most of this is straightforward. The only interesting change is in NetGameLib, where I'm replacing each
s_connslist with a singles_connvariable, because the lists could never contain more than one element.