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

Jump to bottom

Improve player connection and disconnection logic #258

Merged
merged 6 commits into from
Jan 7, 2023
Merged

Improve player connection and disconnection logic #258

merged 6 commits into from
Jan 7, 2023

Conversation

ect0s
Copy link
Contributor

@ect0s ect0s commented Nov 10, 2022 •
edited
Loading

This is an initial port of changes from one of my branches

Fixes new EAC issues, provides a more reliable connection flow.
#257

Fixes Eventstore memory growth
#235

ect0s added 3 commits November 9, 2022 19:46
@ect0s
EventStore Session State
26244c9 
Break eventstore into stateful sub objects,

clearEventStore method for management.
@ect0s
Player Connection/Disconnection Flow. cleareventStore caller
4980916 
pending connection destroyed: error path for clients that fail to connect, required to cleanup connection state in this error case.

client-connected: Supersedes steamid connected, use base unreal engine client flow. Removes dependancy on EAC (broken as of squad 3.4). First step in resolving a new client connection. this gets us a connection id, steamid.

client-login: 2nd step in player connected flow, this setups the steamid into eventstore.steamid-connected.

playercontroller connected: gets us player controller. used in player connected flow.

player-connected: update to use new eventStore layout, now all players should always have suffix,steamid,controller. These are now also cached within the logparser for lookup. last step in player connected flow.

player-disconnected: uses new eventStore layout, marks players in eventStore.disconnected but doesn't remove cached players till map change; may be needed in cases with delayed logs, needs further testing. Broken as of Squad 3.4 due to EAC changes

steamid-connected: Removed in favor of client-connected, no longer works as of squad 3.4 due to EAC changes.
@ect0s
Move events to eventstore.sessions for cleanup
1301c22
@ect0s
Copy link
Contributor Author

ect0s commented Nov 10, 2022 •
edited
Loading

unknown

Gist of the new player connection flow, I understand its hard to follow.

Client Connected appears when a new network connection is made, and contains the steamid.

We set this in eventstore.clients. These events are asyncronous from clients resolving. There is an error case where they get disconnected, pending connection destroyed.

Next, we should see player client login, we use this to pull the steamid from the clients map, followed shortly by player controller connected, which sets up the player controller.

Finally, player connected pulls the current steamid and player controller, plus player suffix, sets these up in eventStore.players, emits the finished event, and does some cleanup.

Other events, particularly those around player damage, are stored in eventstore.sessions.

This allows us to smartly clean these up on NEW GAME events, limiting overall memory growth within the logparser.

@ect0s ect0s changed the title Log parser sessions, fix for 257, 249 Log parser sessions, fix for 257, 235 Nov 10, 2022
@ect0s
Copy link
Contributor Author

ect0s commented Nov 10, 2022

Managed to finish some testing and this should be good.

Michael-Phoenix
Michael-Phoenix suggested changes Nov 17, 2022
View reviewed changes
Copy link

@Michael-Phoenix Michael-Phoenix left a comment

Choose a reason for hiding this comment

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

We removed steamid-connected in this PR
There are however still calls to the corresponding event store that is now empty

squad-server/log-parser/player-connected.js Outdated Show resolved Hide resolved
squad-server/log-parser/player-connected.js Outdated Show resolved Hide resolved
@Michael-Phoenix
Copy link

Thank you for this change! Love the pic to make the flow more understandable!
Fantastic job!

werewolfboy13
werewolfboy13 previously approved these changes Dec 29, 2022
View reviewed changes
Copy link
Collaborator

@werewolfboy13 werewolfboy13 left a comment

Choose a reason for hiding this comment

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

Tested long term on a live server. No issues reported.

@werewolfboy13 werewolfboy13 enabled auto-merge (squash) December 29, 2022 00:21
@werewolfboy13 werewolfboy13 added the core bug Bug related to the core SquadJS API label Dec 29, 2022
Thomas-Smyth
Thomas-Smyth previously requested changes Jan 1, 2023
View reviewed changes
Copy link
Collaborator

@Thomas-Smyth Thomas-Smyth left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me, and I gather this has been tested long-term in production.

A few minor questions and some quick fixes to apply, but otherwise looks good to merge.

Ah, could you also rename the PR to something suitable for the changelog please?

core/log-parser/index.js Show resolved Hide resolved
core/log-parser/index.js Show resolved Hide resolved
squad-server/log-parser/client-login.js Outdated Show resolved Hide resolved
@Thomas-Smyth Thomas-Smyth added the minor Minor Change label Jan 1, 2023
@ect0s ect0s changed the title Log parser sessions, fix for 257, 235 Update Player Connected/Player Disconnected log parser regex after EOS Update. Jan 1, 2023
@ect0s ect0s changed the title Update Player Connected/Player Disconnected log parser regex after EOS Update. Update Player Connected/Player Disconnected log parser regex Jan 1, 2023
@Thomas-Smyth Thomas-Smyth changed the title Update Player Connected/Player Disconnected log parser regex Improve player connection and disconnection logic Jan 2, 2023
@ect0s
Copy link
Contributor Author

ect0s commented Jan 3, 2023

In the process of renaming steamid-connected I had to roll forward to master branch to deal with merge conflicts

But this should be good to go

@ect0s ect0s requested a review from Thomas-Smyth January 4, 2023 03:00
@werewolfboy13 werewolfboy13 dismissed Thomas-Smyth’s stale review January 5, 2023 00:44

Changes have been made, new review is needed.

@Thomas-Smyth Thomas-Smyth disabled auto-merge January 7, 2023 11:46
@Thomas-Smyth Thomas-Smyth merged commit 164bad3 into Team-Silver-Sphere:master Jan 7, 2023
This was linked to issues Jan 13, 2023
SquadRcon.onData Crashing Recently #200
Closed
Possible Memory Leak #249
Closed
This was referenced Jan 13, 2023
Closed
Closed
@ect0s ect0s removed a link to an issue Jan 16, 2023
SquadRcon.onData Crashing Recently #200
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Michael-Phoenix Michael-Phoenix Michael-Phoenix requested changes

@werewolfboy13 werewolfboy13 werewolfboy13 left review comments

@Thomas-Smyth Thomas-Smyth Awaiting requested review from Thomas-Smyth

Assignees
No one assigned
Labels
core bug Bug related to the core SquadJS API minor Minor Change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Possible Memory Leak
4 participants
@ect0s @Michael-Phoenix @Thomas-Smyth @werewolfboy13