Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port NetCli/NetGameLib to standard C++ containers #1487

Merged
merged 6 commits into from
Oct 17, 2023
Merged

Port NetCli/NetGameLib to standard C++ containers #1487

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6193fd9
Remove unused and unimplemented NetCliQueue
dgelessus Aug 20, 2023
5b2cc2e
Replace s_conns lists with single pointers
dgelessus Aug 20, 2023
caee7c6
Convert plNglTrans lists from pnUtList to std::list
dgelessus Aug 20, 2023
86576c5
Remove no longer used pnUtList (LISTDECL, LIST, LINK)
dgelessus Aug 20, 2023
bd6b2b2
Move completed transactions more efficiently with std::list::splice
dgelessus Sep 17, 2023
f931654
Lock s_critsect in AbandonConn to be safe
dgelessus Sep 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 26 additions & 17 deletions Sources/Plasma/PubUtilLib/plNetGameLib/Private/plNglAuth.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ struct CliAuConn : hsRefCnt {
void Send (const uintptr_t fields[], unsigned count);

std::recursive_mutex critsect;
LINK(CliAuConn) link;
AsyncSocket sock;
NetCli * cli;
ST::string name;
Expand Down Expand Up @@ -1249,7 +1248,7 @@ enum {

static bool s_running;
static std::recursive_mutex s_critsect;
static LISTDECL(CliAuConn, link) s_conns;
static CliAuConn* s_conn = nullptr;
static CliAuConn * s_active;
Comment on lines +1251 to 1252
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like a good approach. 👍

static ST::string s_accountName;
static ShaDigest s_accountNamePassHash;
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@Hoikas Hoikas Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, you could just lock s_critsect inside AbandonConn

That seems like the best solution, yeah.

conn->abandoned = true;

conn->StopAutoReconnect();
Expand Down Expand Up @@ -1425,7 +1423,9 @@ static void NotifyConnSocketConnectFailed (CliAuConn * conn) {
{
hsLockGuard(s_critsect);
conn->cancelId = nullptr;
s_conns.Unlink(conn);
if (s_conn == conn) {
s_conn = nullptr;
}

if (conn == s_active)
s_active = nullptr;
Expand All @@ -1444,7 +1444,9 @@ static void NotifyConnSocketDisconnect (CliAuConn * conn) {
{
hsLockGuard(s_critsect);
conn->cancelId = nullptr;
s_conns.Unlink(conn);
if (s_conn == conn) {
s_conn = nullptr;
}

if (conn == s_active)
s_active = nullptr;
Expand Down Expand Up @@ -1525,13 +1527,13 @@ static void Connect (

{
hsLockGuard(s_critsect);
while (CliAuConn * oldConn = s_conns.Head()) {
if (oldConn != conn)
UnlinkAndAbandonConn_CS(oldConn);
else
s_conns.Unlink(oldConn);
if (CliAuConn* oldConn = s_conn) {
s_conn = nullptr;
if (oldConn != conn) {
AbandonConn(oldConn);
}
}
s_conns.Link(conn);
s_conn = conn;
}

Cli2Auth_Connect connect;
Expand Down Expand Up @@ -1643,7 +1645,10 @@ void CliAuConn::TimerReconnect () {

if (!s_running) {
hsLockGuard(s_critsect);
UnlinkAndAbandonConn_CS(this);
if (s_conn == this) {
s_conn = nullptr;
}
AbandonConn(this);
}
else {
Ref("Connecting");
Expand Down Expand Up @@ -4671,8 +4676,10 @@ void AuthDestroy (bool wait) {

{
hsLockGuard(s_critsect);
while (CliAuConn * conn = s_conns.Head())
UnlinkAndAbandonConn_CS(conn);
if (CliAuConn* conn = s_conn) {
s_conn = nullptr;
AbandonConn(conn);
}
s_active = nullptr;
}

Expand Down Expand Up @@ -4769,8 +4776,10 @@ void NetCliAuthAutoReconnectEnable (bool enable) {
void NetCliAuthDisconnect () {

hsLockGuard(s_critsect);
while (CliAuConn * conn = s_conns.Head())
UnlinkAndAbandonConn_CS(conn);
if (CliAuConn* conn = s_conn) {
s_conn = nullptr;
AbandonConn(conn);
}
s_active = nullptr;
}

Expand Down
43 changes: 26 additions & 17 deletions Sources/Plasma/PubUtilLib/plNetGameLib/Private/plNglFile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ namespace Ngl { namespace File {
***/

struct CliFileConn : hsRefCnt {
LINK(CliFileConn) link;
hsReaderWriterLock sockLock; // to protect the socket pointer so we don't nuke it while using it
AsyncSocket sock;
ST::string name;
Expand Down Expand Up @@ -234,7 +233,7 @@ enum {

static bool s_running;
static std::recursive_mutex s_critsect;
static LISTDECL(CliFileConn, link) s_conns;
static CliFileConn* s_conn = nullptr;
static CliFileConn * s_active;
static std::atomic<long> s_perf[kNumPerf];
static unsigned s_connectBuildId;
Expand Down Expand Up @@ -275,8 +274,7 @@ static CliFileConn * GetConnIncRef (const char tag[]) {
}

//============================================================================
static void UnlinkAndAbandonConn_CS (CliFileConn * conn) {
s_conns.Unlink(conn);
static void AbandonConn(CliFileConn* conn) {
conn->abandoned = true;

if (conn->AutoReconnectEnabled())
Expand Down Expand Up @@ -325,7 +323,9 @@ static void NotifyConnSocketConnectFailed (CliFileConn * conn) {
{
hsLockGuard(s_critsect);
conn->cancelId = nullptr;
s_conns.Unlink(conn);
if (s_conn == conn) {
s_conn = nullptr;
}

if (conn == s_active)
s_active = nullptr;
Expand Down Expand Up @@ -355,7 +355,9 @@ static void NotifyConnSocketDisconnect (CliFileConn * conn) {
{
hsLockGuard(s_critsect);
conn->cancelId = nullptr;
s_conns.Unlink(conn);
if (s_conn == conn) {
s_conn = nullptr;
}

if (conn == s_active)
s_active = nullptr;
Expand Down Expand Up @@ -493,13 +495,13 @@ static void Connect (CliFileConn * conn) {

{
hsLockGuard(s_critsect);
while (CliFileConn * oldConn = s_conns.Head()) {
if (oldConn != conn)
UnlinkAndAbandonConn_CS(oldConn);
else
s_conns.Unlink(oldConn);
if (CliFileConn* oldConn = s_conn) {
s_conn = nullptr;
if (oldConn != conn) {
AbandonConn(oldConn);
}
}
s_conns.Link(conn);
s_conn = conn;
}

Cli2File_Connect connect;
Expand Down Expand Up @@ -587,7 +589,10 @@ void CliFileConn::TimerReconnect () {

if (!s_running) {
hsLockGuard(s_critsect);
UnlinkAndAbandonConn_CS(this);
if (s_conn == this) {
s_conn = nullptr;
AbandonConn(this);
}
}
else {
Ref("Connecting");
Expand Down Expand Up @@ -1303,8 +1308,10 @@ void FileDestroy (bool wait) {

{
hsLockGuard(s_critsect);
while (CliFileConn * conn = s_conns.Head())
UnlinkAndAbandonConn_CS(conn);
if (CliFileConn* conn = s_conn) {
s_conn = nullptr;
AbandonConn(conn);
}
s_active = nullptr;
}

Expand Down Expand Up @@ -1379,8 +1386,10 @@ bool NetCliFileQueryConnected () {
//============================================================================
void NetCliFileDisconnect () {
hsLockGuard(s_critsect);
while (CliFileConn * conn = s_conns.Head())
UnlinkAndAbandonConn_CS(conn);
if (CliFileConn* conn = s_conn) {
s_conn = nullptr;
AbandonConn(conn);
}
s_active = nullptr;
}

Expand Down
35 changes: 21 additions & 14 deletions Sources/Plasma/PubUtilLib/plNetGameLib/Private/plNglGame.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ namespace Ngl { namespace Game {
***/

struct CliGmConn : hsRefCnt {
LINK(CliGmConn) link;

std::recursive_mutex critsect;
AsyncSocket sock;
AsyncCancelId cancelId;
Expand Down Expand Up @@ -157,7 +155,7 @@ enum {

static bool s_running;
static std::recursive_mutex s_critsect;
static LISTDECL(CliGmConn, link) s_conns;
static CliGmConn* s_conn = nullptr;
static CliGmConn * s_active;
static FNetCliGameRecvBufferHandler s_bufHandler;
static FNetCliGameRecvGameMgrMsgHandler s_gameMgrMsgHandler;
Expand Down Expand Up @@ -194,8 +192,7 @@ static CliGmConn * GetConnIncRef (const char tag[]) {
}

//============================================================================
static void UnlinkAndAbandonConn_CS (CliGmConn * conn) {
s_conns.Unlink(conn);
static void AbandonConn(CliGmConn* conn) {
conn->abandoned = true;
if (conn->cancelId) {
AsyncSocketConnectCancel(conn->cancelId);
Expand Down Expand Up @@ -244,7 +241,9 @@ static void NotifyConnSocketConnectFailed (CliGmConn * conn) {
{
hsLockGuard(s_critsect);
conn->cancelId = nullptr;
s_conns.Unlink(conn);
if (s_conn == conn) {
s_conn = nullptr;
}

notify
= s_running
Expand All @@ -270,7 +269,9 @@ static void NotifyConnSocketDisconnect (CliGmConn * conn) {
bool notify;
{
hsLockGuard(s_critsect);
s_conns.Unlink(conn);
if (s_conn == conn) {
s_conn = nullptr;
}

notify
= s_running
Expand Down Expand Up @@ -377,9 +378,11 @@ static void Connect (

{
hsLockGuard(s_critsect);
while (CliGmConn * conn = s_conns.Head())
UnlinkAndAbandonConn_CS(conn);
s_conns.Link(conn);
if (CliGmConn* oldConn = s_conn) {
s_conn = nullptr;
AbandonConn(oldConn);
}
s_conn = conn;
}

Cli2Game_Connect connect;
Expand Down Expand Up @@ -722,8 +725,10 @@ void GameDestroy (bool wait) {

{
hsLockGuard(s_critsect);
while (CliGmConn * conn = s_conns.Head())
UnlinkAndAbandonConn_CS(conn);
if (CliGmConn* conn = s_conn) {
s_conn = nullptr;
AbandonConn(conn);
}
s_active = nullptr;
}

Expand Down Expand Up @@ -783,8 +788,10 @@ void NetCliGameStartConnect (
//============================================================================
void NetCliGameDisconnect () {
hsLockGuard(s_critsect);
while (CliGmConn * conn = s_conns.Head())
UnlinkAndAbandonConn_CS(conn);
if (CliGmConn* conn = s_conn) {
s_conn = nullptr;
AbandonConn(conn);
}
s_active = nullptr;
}

Expand Down
Loading