Connection State Changes

PretendoNetwork / nex-go

Barebones PRUDP/NEX server library written in Go

GNU Affero General Public License v3.0

74 stars 16 forks source link

Connection State Changes #65

Closed wolfendale closed 5 months ago

wolfendale commented 5 months ago

Related to #48

Changes:

This makes selective changes to the connection state checks and moves the logic of resetting the connection heartbeat to after we've determined if a packet is valid for the current connection state. ~This means that connections won't be kept alive by invalid traffic.~ I'm no longer convinced the last statement there is true. That change may be redundant as the connection would already be dead and resetHeartbeat wouldn't do anything but it's still probably right that we only allow valid packets to keep a connection alive.

I've added reasoning for each change as comments in this PR

[x] I have read and agreed to the Code of Conduct.
[x] I have read and complied with the contributing guidelines.
[x] What I'm implementing was an approved issue.
[x] I have tested all of my changes.

jonbarrow commented 5 months ago

This probably should have targeted the branch in #48 rather than master, since now we have 2 PRs for the exact same changes. But since that one is kinda outdated now I'll just close it in favor of this one

In the future though if there's already an active pull request for changes you want to make, just PR into that feature branch

wolfendale commented 5 months ago

This probably should have targeted the branch in #48 rather than master, since now we have 2 PRs for the exact same changes. But since that one is kinda outdated now I'll just close it in favor of this one

In the future though if there's already an active pull request for changes you want to make, just PR into that feature branch

This wasn't really intended as a PR that would be merged as much as me trying to get some thoughts on the topic onto GitHub to be discussed. Not sure if there's a better format for that?

jonbarrow commented 5 months ago

This wasn't really intended as a PR that would be merged as much as me trying to get some thoughts on the topic onto GitHub to be discussed. Not sure if there's a better format for that?

That's fair. I can't think fo a better format for this, but that being said if the work to actually implement things is being done then I don't see why it wouldn't be a PR intended for merge?

jonbarrow commented 5 months ago

LGTM. @DaniElectra anything else?