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.

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

network layer reduction #1785

Closed
wants to merge 23 commits into from
Closed

network layer reduction #1785

wants to merge 23 commits into from

Conversation

fartwhif
Copy link
Collaborator

@fartwhif fartwhif commented Apr 23, 2019

  • Combine as many classes as possible without violating KISS or "the pattern"

  • Implement I/O queues

  • Test mass connections and correct shortcomings of the implementation during extreme conditions

  • To prevent flooding, Implement inbound session packet governor and find threshold

    • proportionate to number of sessions?
  • To prevent flooding, Implement ProcessFragment governor and find threshold

    • proportionate to number of sessions?
  • ProcessFragment needs to be a producer for game loop consumer, game loop consumer pull X number of messages to keep server loop tight (500? 1000? more?)

  • S2C Retransmission request fulfillment, or C2S NAK, needs to have complexity added

    • if the packet being retransmitted contains physics data, that physics data needs to be replaced with the latest version sent if the latest version is not the one in the original packet.
  • Outbound queue needs an independent consumer thread, there should be no reason to send via game loop in bursts every game iteration, outbound can be constant

    • process outbound messages to players
    • add messages to outbound packets until 484 size reached or every 66ms
    • continue creating packets until outbound cleared
    • Outbound fragment bundling needs to be reduced to a single collection, as it adds unnecessary complexity at the cost of readability, understandability, and changeability without actually adding any benefit or working towards a required specification or towards an architectural norm. Since outbound queue will be sent by the fluidity of the independent consumer thread combined with the capabilities of the average modern internet connection fragment prioritization by queue or any other need for grouping them together is probably not needed, but I could be mistaken. Comments about this are most welcome.
  • Sessions should only be created upon authentication success, reducing the abuse potential and increasing network layer fitness.

    • the cookie in the cookie exchange can be a constant, eliminating the need for a state machine during authentication, it doesn't actually provide any security or stability related benefits.

    • port 9001 listener can/should be omitted as it adds unnecessary complexity at the cost of readability, understandability, and changeability.

      • without the 9001 listener the client will time out trying to send ConnectionResponse to 9001 at the routing level almost immediately, and the client will enter port 9000 only mode, and then sends it to 9000
    • session keys should consist of IP Address | Port combined into a Unt64, Uint64 where the top32 is the IP and bottom 32 is the port

      • prevents problems observed during authentication due to the race conditions provided by pre-authenticated connections
    • first packet analysis consists of the following:

      1. a 1 btye packet from an IP to port 9000 or 9001 is a valid request. but the parsing of that packet shows its bad so the whole thing is to be tossed
      2. a mutliple connection packet attack from the same clientfrom same port should be ignored and/or flagged for spamming to be banned
      3. a mutliple connection packet attack from the same client on different ports for same account should be flagged for spamming to be banned, but most recent should be honored
      4. bans are IP as well as account based
      5. you check IP ban first then account
      6. before the server acknowodges anything to client it has to run through those checks
      7. and only when the server considers everything ok, then the session is added and the S2C is sent
  • FindOrCreateSession needs to have a lock around the entire body of the function

    • Allows seamless handling of single account, multiple session, during times of great contention

misc fixes:

  • boot command via console causes exception because session is not null in audit call
  • abnormal sequence detection
    • terminate session upon detection

@fartwhif fartwhif requested a review from gmriggs April 23, 2019 04:02
public static void HandleConnectResponse(Session session)
{
if (WorldManager.WorldStatus == WorldManager.WorldStatusState.Open || session.AccessLevel > AccessLevel.Player)
{
DatabaseManager.Shard.GetCharacters(session.AccountId, false, result =>
{
// If you want to create default characters for accounts that have none, here is where you would do it.
if (result.Count == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Think everything in this func isn't meant for master...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

woops, left that in by accident, thanks for the note, i'll remove it.

@@ -45,44 +45,74 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "ACE.Adapter", "ACE.Adapter\
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Copy link
Member

@LtRipley36706 LtRipley36706 Apr 23, 2019

Choose a reason for hiding this comment

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

Not sure if this is needed for FakeClient, but for sure none of the already existing projects/sln should have added AnyCPU build choices as those were specifically removed a year+ ago to force x64 only

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll fix this by removing the extra build configurations, good call


<ItemGroup>
<Reference Include="ACE.Common">
<HintPath>..\ACE.Server\bin\x64\Debug\netcoreapp2.1\ACE.Common.dll</HintPath>
<HintPath>E:\from desktop\ACE\ACE-plugin\Source\ACE.Server\bin\x64\Debug\netcoreapp2.1\ACE.Common.dll</HintPath>
Copy link
Member

Choose a reason for hiding this comment

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

might be an issue here, not sure

Copy link
Collaborator Author

@fartwhif fartwhif Apr 23, 2019

Choose a reason for hiding this comment

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

good call, i'll try to make it relative, had to move the project folder because VS tries to add the project to the main .sln file when the project folder resides here

Copy link
Member

@Mag-nus Mag-nus left a comment

Choose a reason for hiding this comment

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

I mentioned this in DM's, but you can make it a lot easier for us to review this PR by creating a dummy Session.Network object that simply relays EnqueueSend down to the base session.

It would leave the vast majority of these files unchanged so we can review the actual changes more clearly.

Then, after the architecture change is approved and merged, the wrapper class can be removed.

@Mag-nus
Copy link
Member

Mag-nus commented Apr 23, 2019

Can you please describe to us what this PR accomplishes?
What does it fix/resolve?
What does it improve?
Does it help you with further architecture plans? If so, how?

@fartwhif
Copy link
Collaborator Author

fartwhif commented Apr 23, 2019

I mentioned this in DM's, but you can make it a lot easier for us to review this PR by creating a dummy Session.Network object that simply relays EnqueueSend down to the base session.

It would leave the vast majority of these files unchanged so we can review the actual changes more clearly.

Then, after the architecture change is approved and merged, the wrapper class can be removed.

I'll see if it can't revert the reduction and do it right. Good call!

@fartwhif fartwhif self-assigned this Apr 23, 2019
@Mag-nus
Copy link
Member

Mag-nus commented Apr 23, 2019

I'm starting to go over your checklist, thank you for providing that.

The first thing I notice is the combination of Session and NetworkSession. I was aware that you had done this, which is why I suggested the wrapper, but I was curious as to why.

The reason for combining class does not apply here. Session and NetworkSession should be kept functionally separate.

NetworkSession is designed to handle low level packet work. This includes:

  • Packet processing
  • Packet transmission

Session is designed to handle higher level session management. This includes:

  • Dispatching received packets (through to NetworkSession)
  • Processing packet transmit requests (through to NetworkSession)
  • Managing server/client/world states
  • Managing the session:player relationship

@fartwhif
Copy link
Collaborator Author

fartwhif commented Apr 23, 2019

I'm starting to go over your checklist, thank you for providing that.

The first thing I notice is the combination of Session and NetworkSession. I was aware that you had done this, which is why I suggested the wrapper, but I was curious as to why.

The reason for combining class does not apply here. Session and NetworkSession should be kept functionally separate.

NetworkSession is designed to handle low level packet work. This includes:

* Packet processing

* Packet transmission

Session is designed to handle higher level session management. This includes:

* Dispatching received packets (through to NetworkSession)

* Processing packet transmit requests (through to NetworkSession)

* Managing server/client/world states

* Managing the session:player relationship

i'll revert that reduction,

there's one piece around all that, Network Management, manages the session collection, does session discrimination, and does low level network I/O, not sure how much that deviates from "the pattern".

@fartwhif fartwhif closed this Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants