Some minor cosmetic fixes. #147
Conversation
Some possible threading issues fixed.
Source/ACE/Network/NetworkSession.cs
Outdated
| } | ||
|
|
||
| public void HandlePacket(ClientPacket packet) | ||
| { | ||
| lastReceivedSequence = packet.Header.Sequence; | ||
| lock (lockObject) |
There was a problem hiding this comment.
I really don't see the need for this lock. If stuff has to Enqueue as a response, it will hit those locks accordingly. This lock could significantly degrade performance.
Source/ACE/Network/NetworkSession.cs
Outdated
| @@ -12,110 +13,124 @@ namespace ACE.Network | |||
| { | |||
| public class NetworkSession | |||
| { | |||
| private readonly Object lockObject = new Object(); | |||
There was a problem hiding this comment.
I strongly prefer lock objects to be "[objectBeingLocked]Lock" and directly adjacent to it. This appears to be a functional lock for both HandlePacket and EnqueueSend, and I dont' see the necessity of those sharing a lock. In this case, I would have named it "currentBundleLock" or "currentBundleMutex" - either are good.
| @@ -168,24 +181,22 @@ private void FlushBundle() | |||
|
|
|||
There was a problem hiding this comment.
FlushBundle, above, needs a lock but GitHub won't let me comment on it since you didn't change it. It's what actually wipes out the bundle and resets it. If this is being called within a lock, then it really should be doc'ed to state that the lock is a requirement for calling it. If it's only being called in one place, just move the code to where it's called in the lock.
Source/ACE/Network/NetworkSession.cs
Outdated
| } | ||
|
|
||
| public void EnqueueSend(params ServerPacket[] packets) | ||
| { | ||
| foreach (var packet in packets) | ||
| PacketQueue.Enqueue(packet); | ||
| lock (lockObject) |
There was a problem hiding this comment.
this needs to be a different/new lock/mutex - "packetQueueLock" or "packetQueueMutex" would both be good.
Source/ACE/Network/NetworkSession.cs
Outdated
| @@ -168,24 +181,22 @@ private void FlushBundle() | |||
|
|
|||
| private void FlushPackets() | |||
| { | |||
| while (PacketQueue.Count > 0) | |||
| while (packetQueue.Count > 0) | |||
There was a problem hiding this comment.
needs to lock packetQueueLock and copy out the items to be sent and/or "new" up the queue from within the lock, then continue.
| @@ -342,7 +353,7 @@ private void SendBundle(NetworkBundle bundle) | |||
| } | |||
| } | |||
|
|
|||
| PacketQueue.Enqueue(packet); | |||
| packetQueue.Enqueue(packet); | |||
There was a problem hiding this comment.
needs to lock "packetQueueLock"
Some possible threading issues fixed.
This also forces the build for ACE to x64 only.