Network Patch

Dec 12, 2007 at 7:47 PM
Im uploading a snapshot of my work on the network, mainly just to prove im doing something, and to get some feedback.

It sonly a preview of a preview, so I am well aware there are a number of issues to address. The ones I will sort out next are:

  • Comment the methods
  • Add systemlink and live
  • Extensively test
  • Streamline its linking with the message system
  • Simplify certain methods

At the minute ive modifed the sample to create a message each loop to check if the network gets it. It should output two lines, one server one client. At the minute customserver does nothing, as its only meant as a base class for modders to be loaded via the config manager. Ill add this when weve decided ona format for it.

Hope its ok so far, but it needs a lot of work. Any comments?

Also dont apply to your main checkout as its not complete. It should install fine, but the server wont give me a fresh checkout to check it.
Dec 13, 2007 at 2:01 AM
Nice start!

Few comments:
  • You know you can switch directly on enumerated values, right? Though, in this case, I'm not sure switching on the network type is the right way of doing this. Why not just let a client pass in an INetworkSubSystem reference, and query it for whatever you need (i.e. local flag). That way, clients can create their own network handlers without having to change the base network code.
  • I'm guessing the CustomHandler interface is only used for local communication, and an INetworkSubSystem is only used for network communication? If so, why isn't CustomHandler virtual, or an interface?
  • I'm actually kind of confused on the local vs. non-local network types, especially in NetworkSystem.Update. Is it just an optimization to prevent having to go through a loop-back connection?
Dec 13, 2007 at 5:00 AM
I also have a few comments:
In NetworkSystem.cs
  • Remember to remove the using statements you are not using (System.Text)
  • Move the enumeration outside the class, it just gets annoying typeing NetworkSystem all the time
  • Remember to use "this" prefix on instance members
  • Remember to use camelCase on fields
  • On the construcor loose the word Instance, it's implicit
  • You should rename GameMessage to RecieveMessage as that's what's it's doing
  • On InitializeNetwork loose the Network as it's implicit
  • getAvailableSessions should be a property (get/set methods should be avoided)
  • NetworkSystem should implement IUpdateable
  • GenericAvailableNetworkSession should have it's own file, it's not a private type
  • GenericAvailableNetworkSession should just be NetworkSession because generic has a different meaning in C#, also it could be any session
  • Rename gamemessages to messages, it would work even if it wasn't a game

In CustomHandler.cs
  • I think this should be renamed to NetworkHandler and be an interface. There would then be to classes implementing it ServerHandler and ClientHandler
  • HandleMessages should be renamed to HandleMessage as only on message can be passed

In INetworkSubSystem
  • I really think that the wording SubSystem is wrong, lets find another; INetworkSystem is acceptable I think
  • I think that you are missing LeaveSession
  • SendCustomMessage should just be SendMessage
  • The interface should implement IUpdateable
Dec 13, 2007 at 12:18 PM
Right, lots of comments, time to reply!


shawmishrak wrote:
Nice start!

Few comments:
  • You know you can switch directly on enumerated values, right? Though, in this case, I'm not sure switching on the network type is the right way of doing this. Why not just let a client pass in an INetworkSubSystem reference, and query it for whatever you need (i.e. local flag). That way, clients can create their own network handlers without having to change the base network code.


I like that solution more, as mine is inelegant at best. Also mean I can remove CustomHandler as that was going to be the game specific part. This way they extend the subsytems Local, Live etc and just pass in those. I want a better way then using a flag for local, so I may work on a subsytem for it. In the meantime we have InitializeNetwork. Im goign to break out local and have InitializeLocalNetwork as well. Simpler that way.


shawmishrak wrote:
  • I'm guessing the CustomHandler interface is only used for local communication, and an INetworkSubSystem is only used for network communication? If so, why isn't CustomHandler virtual, or an interface?


Going to remove it. At the minute, Custom handler applies to all. It is a way for inserting game specific code in. That would have been more clear if id mocked up systemlink as well.


shawmishrak wrote:
  • I'm actually kind of confused on the local vs. non-local network types, especially in NetworkSystem.Update. Is it just an optimization to prevent having to go through a loop-back connection?


Cant remeber as im afk at the minute. I need to rework local as its a bit of a kludge. Ill repost on this when I get home.
Dec 13, 2007 at 12:24 PM


Sturm wrote:
I also have a few comments:
In NetworkSystem.cs
  • Remember to remove the using statements you are not using (System.Text)
  • Move the enumeration outside the class, it just gets annoying typeing NetworkSystem all the time
  • Remember to use "this" prefix on instance members
  • Remember to use camelCase on fields
  • On the construcor loose the word Instance, it's implicit


I have a lot of tidying to do I know, but I wanted to get a functional preview up for comments. No point getting good looking code that has to be entirely redone!


Sturm wrote:
  • You should rename GameMessage to RecieveMessage as that's what's it's doing
  • On InitializeNetwork loose the Network as it's implicit
  • getAvailableSessions should be a property (get/set methods should be avoided)
  • NetworkSystem should implement IUpdateable
  • GenericAvailableNetworkSession should have it's own file, it's not a private type
  • GenericAvailableNetworkSession should just be NetworkSession because generic has a different meaning in C#, also it could be any session
  • Rename gamemessages to messages, it would work even if it wasn't a game


Sure thing on these. Its called generic at the moment as I cant call it NetworkSession without it conflicting with Xna.Netowrk. Can anyone think of an obvious name thats not already in use?


Sturm wrote:
In CustomHandler.cs
  • I think this should be renamed to NetworkHandler and be an interface. There would then be to classes implementing it ServerHandler and ClientHandler
  • HandleMessages should be renamed to HandleMessage as only on message can be passed

Im going to remove it in all likelihood and merge it into each sub system.


Sturm wrote:
In INetworkSubSystem
  • I really think that the wording SubSystem is wrong, lets find another; INetworkSystem is acceptable I think
  • I think that you are missing LeaveSession
  • SendCustomMessage should just be SendMessage
  • The interface should implement IUpdateable


My bad on forgetting Leave! Il implement IUpdateable as its better. SendCustomMessage is important as it forces a message to be sent immediately, bypassing the message stack. As such I will leave it in, in case its needed, but rename it as SendImmediateMessage, or something like that. Thanks for all the comments, and keep them coming please!
Dec 13, 2007 at 3:00 PM

shawmishrak wrote:
  • I'm actually kind of confused on the local vs. non-local network types, especially in NetworkSystem.Update. Is it just an optimization to prevent having to go through a loop-back connection?


Home now. Yes its just an optimization. I see no point in using a loop back if I can pass the message stright to the server component directly. Also, i was told Local should not touch the network even remotely, so this kind of ruled out a loopback. I need to really modify local as its inelegant and im not happy with it. Overall functionality should be the same, I just want it more consistent with the other systems.
Dec 15, 2007 at 3:38 AM
There are some new networking samples for XNA 2.0. You might want to check them out: http://creators.xna.com/Education/Samples.aspx. Theres some on client/server and peer-to-peer architecture.
Dec 16, 2007 at 5:59 PM
Cheers for the heads up, I hadnt noticed yet. Dont expect anything from me now till next weekend, as ill check my code with the samples to see if their more efficient. Im still progressing though!
Dec 17, 2007 at 3:15 PM
Sturm, is the message system thread-safe? I was wondering as I think ill make the entire network system run in a seperate thread, as its simpler yet still should give the performance gain. I need to test this though, but I wanted a rough idea first.
Dec 17, 2007 at 4:59 PM
It should definitely be a requirement that the messaging system be thread-safe. Further, I'd say most public interfaces for engine systems should be thread-safe.
Dec 18, 2007 at 5:30 AM
Yes the messaging system is (or will be) thread-safe.
Dec 22, 2007 at 5:32 PM
Okay, major desing point I want feedback on. To use the network you need to access things like:

  • Search/Join/Leace/Create session functionality
  • End Session
  • Set state as ready
  • Begin session
  • etc

My question is: How should the user access these? I have 3 ides at the minute:

  • As they create the subsystem instance (ill explain why if asked, but its a bit long winded if not neccessary) the keep a referance to it and call methods on it directly
  • Call methods on the subsystem manager, directly or thorugh the Game class
  • Send engine messages that the Network recognises and interprets as special ones.

I dont like ideas 1 or 2 as they arent particularily thread safe. 3 is slower, but these arent the kind of things that need no delay, a few milliseconds shouldnt be a problem, I reckon. Also it would be thread safe, like Sturm says above. That my preferance, but what do others think?

Also, how should it be updated? Do i add a referance in QSGame.Update() or add it as a service/component? I think add to Update as its simpler.
Dec 22, 2007 at 6:48 PM
PLease reply if you read this, I would like to have it ready for the new year.
Dec 22, 2007 at 7:11 PM
Internally, your code should be thread-safe so 1 or 2 wouldn't cause issues. Messages should work fine and help to decouple the systems a bit.

Just write an Update(GameTime) method, and then it doesn't matter. We can either code it to be called in Game.Update(), or inherit your class from IUpdateable and register it as a component.
Dec 22, 2007 at 8:03 PM
Ill try to get it thread safe, as it should be I suppose. Ill go with the Update method and messages. At the moment ill add the necessary code to QSGame to get it to work, but when I upload the patch feel free to change that aspect. Ill just add it to Update for the minute. Cheers for the reply Shaw
Jan 2, 2008 at 5:59 PM
The nore work I do, the unhappier I become. Im not sure if tying it into the message system is a good idea, as it means the engine is ingorant of the message content. This means I cant write any optimization code, based on message type, target extra. All I can do is forward it. We could carry on with this, but itd be difficult to get the data anywhere near small enough fro internet play.

Instead I propose two alternatives:

  • A send method with a byte parameter, and a delegate on the other end for custom handling. This means all optimization can be done outside of the network, in user code.

  • Add a stack in scene manager that holds objects that must be synchronized. The network would automatically synchronise posistion, rotation etc., and optimizations could be done on this.

Any comments/suggestions/flaming?
Jan 2, 2008 at 6:53 PM

A send method with a byte parameter, and a delegate on the other end for custom handling. This means all optimization can be done outside of the network, in user code.


How would you see this working out? Would each system have a networking callback method and each would pick out messages they were responsible for?


Add a stack in scene manager that holds objects that must be synchronized. The network would automatically synchronise posistion, rotation etc., and optimizations could be done on this.


While intriguing, I don't like the idea of creating an inter-dependency between the scene manager and the network layer. However, I could maybe see this working if the network layer really sits between the scene manager and the messaging system.
Jan 2, 2008 at 6:55 PM

mikelid109 wrote:
The nore work I do, the unhappier I become. Im not sure if tying it into the message system is a good idea, as it means the engine is ingorant of the message content.


The engine itself has to be message type agnostic otherwise adding new messages would mean changing the core engine, which is not a option for users of the engine, they should be expanding the functionallity not rewriting it.


mikelid109 wrote:
This means I cant write any optimization code, based on message type, target extra. All I can do is forward it. We could carry on with this, but itd be difficult to get the data anywhere near small enough fro internet play.


The data carried with the messages are defined in the message, so if you want to just have a byte array you would just create a new message like this:

NetworkMessage : Message<Byte[]> 
This should be enough for sending messages. Could you give an example of what you are trying to accomplish?

mikelid109 wrote:
Instead I propose two alternatives:

  • A send method with a byte parameter, and a delegate on the other end for custom handling. This means all optimization can be done outside of the network, in user code.


This should be easilly be implemented using a custom type based on message, so you should be able to do this already.


mikelid109 wrote:
  • Add a stack in scene manager that holds objects that must be synchronized. The network would automatically synchronise posistion, rotation etc., and optimizations could be done on this.


I think that this isn't a good idea, basically every entity should be handled by the server. At least all physics updated entities should be updated through the server. Could you give some examples of entities which shouldn't be updated throught the server?
Jan 2, 2008 at 7:06 PM
Along a tangent line of thought: who will be responsible for sending the updated position/rotation data across the network every X ms, the physics simulator or the scene manager? While intuitively the server's physics simulator should be responsible for the updates, I think this should be the scene manager's job. We've already agreed that there's too much of a data throughput requirement to use messaging for the physics <-> scene manager interaction, and the scene manager and physics simulator may be interdependent in very complicated ways.
Jan 2, 2008 at 7:07 PM
Edited Jan 2, 2008 at 7:09 PM
I understand accessing the byte of the message, but that is still too vague. I would like many types of optimization such as message merging, compression etc depend on knowing at least the primitives type (bool, string) and some require knowledge of the message data ( compressing two update mesages into one). If they are just byte data, I can see know way to do this. If you can offer an alternative I would be more than happy to adopt it, as I want to stay with the message approach for consistency.

All things would be server handled, merely the update message is sent to the server (like on forward key press, the event is passed to the server which then handles the key press for example). That means a client cant be hacked so pressing forward means you move 10x what you should.

Shaw, you know these systems better than me, so Ill leave that up to you.
Jan 2, 2008 at 8:46 PM

mikelid109 wrote:
I understand accessing the byte of the message, but that is still too vague. I would like many types of optimization such as message merging, compression etc depend on knowing at least the primitives type (bool, string) and some require knowledge of the message data ( compressing two update mesages into one). If they are just byte data, I can see know way to do this. If you can offer an alternative I would be more than happy to adopt it, as I want to stay with the message approach for consistency.


Message mergine is very dangerous and very much depends on the messages being send, ofte the order of the messages are important (like doing rotation before translation), this kind of optimization should be done by the game developers as they would have the best idea of what to merge for that specific game.

Compression should be doable nomatter what type of message is send, as we will not know what type of data is being transmitted through the system.

If you could provide an example of what you are trying to acomplish I can try to give you a possible solution to it.


mikelid109 wrote:
All things would be server handled, merely the update message is sent to the server (like on forward key press, the event is passed to the server which then handles the key press for example). That means a client cant be hacked so pressing forward means you move 10x what you should.


I think that is handled where should be handled by the game implementation, we should not impose any restrictions in this matter, as some games have perfect valid scenarios for having code executed on the client, as the load on the server would be too much if it should handle all calculations. Also it's actually not preventing tampering, as I can still tamper with the messages send and recieved.
Jan 3, 2008 at 3:00 PM
By message merging I meant instead of sending 4 translation vectors, its combined to one for example. If you feel its best left to users, Im happy to do so. Have a look here for some compression techniques (Thanks Ikon, great blog) http://blogs.msdn.com/shawnhar/default.aspx. These manily seem to require knowledge of types. The user code do this, but I thought converting it in engine would remove that hassle for the user. What do you think?

I misrepresented what I meant about client/server. I should explain it properly!

I want tro minimise traffic, and as computers are rule based we can make assumptions. If we need a random number, if all share the same seed, they should produce the same 'random' result meaning the event doesnt need to be sent, only the original seed when the game starts. Also if we only send user input to all clients, they can independantly process it. For example:

This way, a user pressing fire requires only a single small message. All resulting explosions/collapsing doors/whatever are computed individually, but should still be consistent.

The other way would be to send the results of the action. This would guarentee consistency, but mean a larger data load.

All sensitive actions (like a gun fired) could be server handled to let it act as the judge. This could be via a special message range, or a message flag.

What does everyone think?
Jan 3, 2008 at 5:29 PM
I don't see how that kind of merging can reliably be done in the network layer. There are too many variables. I thought you meant combining different messages into a single packet to save on packet transmission overhead.
Jan 3, 2008 at 5:51 PM
Edited Jan 3, 2008 at 6:03 PM
That too :)

I was classing that under compression, but its obviously merging now I think about it. Thats were I need to have access to the data type.

Sturm, at the minute im using the IMessage interface, but this doesnt provide me with access to anything. What class should I use for a genral message, that still provides access to the data?

Also, the problem with my suggestion is reliability. If a message is lost, then all later messages become out of sync. I would either need to do a regular full system update, or use a reliable protocol (eg TCP). Would this be too slow do you think?
Jan 3, 2008 at 8:01 PM
TCP would be too slow in general. It would be fine for turn-based games or very simple games, but not for anything more complicated.

I would suggest implementing a serialize/deserialize mechanism within the message system, so an arbitrary message can be serialized to a byte array, and deserialized into a general message, given the target type stored in the byte array somewhere.

You will need to implement some sort of reliability mechanism within the network layer. Any messages that are marked as "reliable" or "in-order" must be processed as such, using acknowledgement packets and packet numbering.
Jan 3, 2008 at 8:40 PM
I had a feeling tcp would be too slow. I will handle reliable myself then. I considered Serialising as the best option, but if I serialized IMessage, would it serialize out correctly? Ill wait for Sturm to get back to me, but I think I may have to try sreialization.
Jan 3, 2008 at 8:47 PM
You and Sturm will need to work out some way of serializing messages. I'm sure it'll involve trade-offs in both of your systems. The simplest way would be to implement Serialize and Deserialize methods for all message classes.

Just make sure you don't use Reflection, it would be way too slow for real-time network communication.
Jan 4, 2008 at 5:59 AM
I'm not sure I understand what you need, when sending messages you would just serialize the message using XmlSerializer or our own as BinarySerializer does not exist on the XBox (see this thread IDisposeable and ISerializeable).

You shouldn't implement IMessage directly, instead inherit from Message<T> which will provide a public T Data property. Also remember that you have to use ObjectPool.Aquire<T>() in order to instantiate your messages, as otherwise the message system will fail (as of patch 654).
Jan 4, 2008 at 9:16 AM

Sturm wrote:
I'm not sure I understand what you need, when sending messages you would just serialize the message using XmlSerializer or our own as BinarySerializer does not exist on the XBox (see this thread IDisposeable and ISerializeable).

You shouldn't implement IMessage directly, instead inherit from Message<T> which will provide a public T Data property. Also remember that you have to use ObjectPool.Aquire<T>() in order to instantiate your messages, as otherwise the message system will fail (as of patch 654).


Im definitely not using XMLSerializer, as I believe this would lead to a larger message size. Xbox assumes we have a max thorughput of 8kb/s, with aorund 7kb/s of that going to voice! As such I only have around 1kb/s to play with, more if we dont support voice. Even with windows sockets Im using this limit, as if its the industries estimate of reasonable, its going to be more accurate than my guess-timate.

I dont follow you sorry. EngineMessageHandler passes an IMessage, so how would I get at the Message behind it? Im not trying to inherit (at least I dont think I am...) I merely need it as a method parameter. eg.

SendMessage(Message m)
{
Byte[] d = m.GetData()
SendData(d)
}
(pseudo code)
Jan 4, 2008 at 10:01 AM
Please remember to use {{ }} around your code.

Currently IMessage only has the Type property. You will need a Serialize method as well. You do not need to know the runtime type of the message, the Type parameter should be enough. You would then do:

        /// <summary>
        /// Sends messages to the system
        /// </summary>
        /// <param name="message">The <see cref="IMessage"/> which should be send</param>
        /// <remarks>Use <see cref="Message<T>"/> when sending errors</remarks>
        public void SendMessage(IMessage message)
        {
            if (message.Type == MessageType.Shutdown)
            {
                this.exiting = true;
            }
 
            if (this.exiting == true)
            {
                return;
            }
 
            if (NetworkManager.Instance.IsNetworkMessage(message) == true)
            {
                NetworkManager.Instance.SendMessage(message);
                return;
            }
 
            // Lock the message queue before adding message
            lock (QSGame.messageLock)
            {
                this.queuedMessages.Add(message);
            }
        } 
In NetworkManager the SendMessage would then look something like this:

        /// <summary>
        /// Sends messages to the network
        /// </summary>
        /// <param name="message">The <see cref="IMessage"/> which should be send</param>
        /// <remarks>Use <see cref="Message<T>"/> when sending errors</remarks>
        public void SendMessage(IMessage message)
        {
            byte[] data = message.Serialize();
            this.SendData(message.Type, data);
        } 
The type information is also passed as you will need this when deserializing the data on the other end. (Remember that Type is int so it takes up a few bytes, be aware of 32/64 bit issues)

Your RecieveMessage could look something like this:

        /// <summary>
        /// Recieves messages from the network
        /// </summary>
        /// <param name="data">The <see cref="byte[]"/> with the data</param>
        public void RecieveMessage(byte[] data)
        {
            int type = this.GetTypeFromData(data);
 
            Type type = this.GetMessageType(type);
            IMessage message = Activator.CreateInstance(type) as IMessage;
            message.Deserialize(data); // data here is without the initial type information
 
            // Do with the message whats needed and might call Game.SendMessage or NetworkManager.SendMessage with the proper response
        } 
This should hopefully help a bit. Questions? Shaw, I can't seem to be avoiding Reflection all together and can't really see how you would go around avoiding that.
Jan 4, 2008 at 2:23 PM
Thats awesome, thanks Sturm. Do you want to handle serialization, or shall I add it? Ill get on with the system, and post back if I need some more clarification.
Jan 4, 2008 at 2:56 PM
Calling Activator.CreateInstance a handful of times per frame has me a little worried. The runtime has to parse type information, instantiate a new object (garbage!), enforce security constraints, etc. Unlike using "new", there's no guarantee that the type even exists, so I'm sure there's extra processing there.

Isn't there a way to tie this into the message pool? Something like ObjectPool.Acquire<type>()? I'm not sure if C# allows template parameters to accept Type objects, but there could be an override like ObjectPool.Acquire(Type t). That way, we can properly recycle messages and not have to worry about the runtime constantly creating dynamic objects.
Jan 4, 2008 at 4:30 PM
Edited Jan 4, 2008 at 4:31 PM
Temporary side note, for Socket host searching im using UDP multicasting, on group 225.48.68.43 and UDP port 1025. Basically a random choice but checked against the wikipedia list http://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers. Defined in QSConstants, so can be changed per program. Any objections?
Jan 4, 2008 at 5:18 PM
You are absolutely right Shaw, I don't know what I was thinking (prob not at all). The Recieve message should look more like this:
  /// <summary>
        /// Recieves messages from the network
        /// </summary>
        /// <param name="data">The <see cref="byte[]"/> with the data</param>
        public void RecieveMessage(byte[] data)
        {
            int type = this.GetTypeFromData(data);
 
            IMessage message = Message.FromType(type) as IMessage;
            message.Deserialize(data); // data here is without the initial type information
 
            // Do with the message whats needed and might call Game.SendMessage or NetworkManager.SendMessage with the proper response
 
            // If the message hasn't been passed to SendMessage then we need to release it ourself.
            if (Passed to SendMessage == false)
            {
                ObjectManager.Release(message);
            }
        } 

I will implement the Message.FromType(int messageType) in a later checkin, until then you can just create a mockup.
Jan 4, 2008 at 7:20 PM
Is it bad practise (or possible?) to use events across two threads?
Jan 4, 2008 at 7:50 PM
Edited Jan 4, 2008 at 8:15 PM
Hi, all

read the next post, please.

Jan 4, 2008 at 7:52 PM
Hi, all

Firstly i like congratulaty all of you that work in this project it's a fantastic.

about the NetworkSystem

would be great if:

- it build in layers, botton-up, provide more complex and easy to use services on top layers, like:

- - receive and send data on low level layer and protocol type like (UDP, TCP) about work asynchronously with ThreadPool.QueueUserWorkItem to processing messages to receive or send. this Layer only receive/send messages from/to Queue, this queue would be call Channel used to communication.

- - - Next layer will be provide all communication patterns like: Request/Response messages for ping services, OneWay message used for communication that need a response like a UDP, this layer also must implement a communication model like: P2P, Client/Server. This layer must be provider only behavior methods to next level services in agree with communication patterns, like: P2P using one way pattern, would have messages like: Update(used by send and receive data), Join, Leave, Ping;


- Message builder to allow that user create own message him self, using C# attributes to define message fields, like: header, body, etc.). The message builder would be a translator between low level network services and high level services like position service, chat, streaming for audio and video services,

- Next layer must provide a service helpers like: security with encryptation using PkCS, etc, serialization for messages.

- - - Next layer would have component services like: audio/video streaming, chat, etc.

sorry my very poor English, i hope to help.



Jan 4, 2008 at 7:52 PM
The C# event keyword kind of events, or the process/thread synchronization concept of events?

If you mean the C# keyword kind, then I don't see why not as long as the methods hooked up to the events are thread-safe.
Jan 4, 2008 at 8:55 PM
I may have found an alternative. Its simpler, but uses depreceated methods (namely thread.suspend and thread.resume). Does anyone have a problem with using these as a temporary measure, with a view to getting a replacement in for the next version?
Jan 4, 2008 at 9:29 PM
mikelid109, what are you trying to accomplish? Explicitly suspending and resuming threads should not be used for thread synchronization.

adribeiro, those are all very good ideas! I know mikelid109 is working on the asynronous network messaging. The network layer and messaging system will be sort-of tied together. It would be hard to define attributes for message parts. That's probably best left to the network layer. I can't think of any scenarios where users would need to explicitly say what goes in the header and what goes in the body. Also, parsing attributes at run-time is somewhat costly and it would be hazardous to performance if we had to process C# attributes for every message sent.

The way it's set up now (or will be in the first release) is that clients are free to build their own messages, which get passed around internally on the client-side. These messages can also be flagged for network broadcast, and the network layer will take care of transmitting them however it sees fit to the server.

P2P processing is a possibility of the future. The problem there lies with security, and deciding how to resolve disputes. If each client was wholly responsible for their own actions, it would be too easy to hack a game client to cheat.

We need to be very careful with encryption. Eventually all game data should be encrypted, or at least include an encrypted hash of the entire body to prevent third-party tampering. But we also need to make sure text data and anything that could be used as person-to-person communication is not encrypted to comply with international laws.

The idea of creating several network layers on top of raw sockets is an interesting idea. I would encourage you and mikelid109 to talk more on the topic and see what you two can come up with. I can see where different clients may want different levels of access to the networking functionality, and it's definitely something to consider for future versions.
Jan 4, 2008 at 9:31 PM
Why do you need to suspend a thread? If you do you should use one of sub classes of WaitHandle instread of Suspend.
Jan 5, 2008 at 5:56 AM
Hi mikelid109,

I have a partial implementation of asynchronous communication using AsyncResult derived class and ThreadPool.QueueUserWorkItem to receive and send data. Tha solution is quity simples and my idea was try eliminate block time of Socket is don't listen because is processing a UDP package. I did this using two levels of asynchronous operations one using a default implementation provide by Socket.BeginReceive to get bytes from the network device, put on the queue withouth any process and one thread that use a ThreadPool.QueueUserWorkItem to process the deserialization of queue bytes in real user messages and put on channel that can read by user.

I think this solution can be use for TCP too, with a few changes. This solution is based on Proactor Pattern used by ACE network framework for C++ applications.

I can send my code to you, if you want.


Jan 5, 2008 at 11:01 AM
Edited Jan 5, 2008 at 11:18 AM
I would be really interested in having a look at your code, you can contact me thorugh my profile. It would be interesting to have a look at and fit in. As for mulitple layers, it should be possible, as its designed to be extendible. For example to encrypt you could extend SocketSubSystem, override the SendData method with code to read messages of the queue to be sent, encrypt them then push them back. Calling base.Update() would then send these, I believe.

As for the suspension of the thread, Im trying to recycle a thread to save it being recreated, but I only need one so thread pool is also inefficient. For example I would suspend it if the game quits to the main menu, and resume (with some tinckering) when they join a new session. The network thread basically handles the network update, and very little else. All session joining etc is done on the main thread. ANyway, ive found a simpler way of using a static bool that it checks each loop, with if its false it sleeps to reduce wasted cpu cycles.

Speaking of which, how would people like to interface with the network? Should I have a public function (JoinSession) or a custom message type, the when persed calls a private JoinSession? I prefer the message option as for SubSystem.JoinSession to be called would require a wrapper in NetworkSystem, and possibly QSGame.
Jan 5, 2008 at 2:22 PM
Ah, so you're really trying to achieve thread communication. Are you familiar with thread sychronization primitives like semaphores and mutexes? A simple extension of these ideas are C#'s ManualResetEvent and AutoResetEvent. They're an easy way to send signals between threads. For instance, the worker thread can block on an event, and the main thread can set the event whenever it needs work done.
Jan 5, 2008 at 5:56 PM

shawmishrak wrote:
Ah, so you're really trying to achieve thread communication. Are you familiar with thread sychronization primitives like semaphores and mutexes? A simple extension of these ideas are C#'s ManualResetEvent and AutoResetEvent. They're an easy way to send signals between threads. For instance, the worker thread can block on an event, and the main thread can set the event whenever it needs work done.


I was aware of AutoResetEvent, but decided against it. Firstly, I wanted the meain thread to stop and resume the worker, not the worker to pause when done (although there was a workaround I considered), secondly, it was much simpler to use those methods. Like I said, its unimportant now, as Im now using a static variable, whihc appears to work fine.
Jan 6, 2008 at 11:08 AM
If you need to do thread sync you need to use WaitHandlers, using a static bool is not a good solution as most often this need to be public and then there is no real sync safety as everyone can continue the waiting thread.
Jan 6, 2008 at 2:48 PM
Why do you need to stop the thread instead of pausing it? Stopping and starting threads is more overhead than just pausing them when not used. The Windows scheduler knows what it's doing and can do this very efficiently. If a thread is waiting on a WaitHandle, it doesn't get CPU time.
Jan 7, 2008 at 7:59 PM
Ive uploaded a patch for comment. It snot perfect, yet I want any bugs ironed out before I submit the version for the final commit. Its shows the basic outline of the system, and an example local subsystem. I still need to implement WaitHandle, but that is on its way!
Jan 7, 2008 at 9:08 PM
Looks like a great start so far!

Where are the comments? :) They really do make it a lot easier to understand code. A lot of times it's hard to guess exactly what a method is supposed to do with just the name and names of parameters. For instance, CustomUpdate and SendCustomMessage. What are their purpose?

Also, especially since you rely on threading, it would be good if each method comment contained a line about which thread calls the method, the main processing thread or a worker thread.

Everywhere:

Why do you use extra objects for locking? i.e.

lock (AbstractNetworkSubSystem.currentStateLocker)
{
    subSystem.CurrentState = (int)State.FindingSession;
}

Is there a reason you're not locking on the object you're going to use, like subSystem.CurrentState?


LocalSubSystem.cs:

This is very dangerous:

public override Stack<IMessage> recievedMessages
{
    get { lock (AbstractNetworkSubSystem.recievedMessagesLocker) { return recievedMessagesStack; } }
    set { lock (AbstractNetworkSubSystem.recievedMessagesLocker) { this.recievedMessagesStack = value; } }
}

For the getter, you're only holding the lock for the retrieval of the stack instance. Once the client code gets the stack reference, the lock is no longer active. And for the setter, why not lock on recievedMessageStack. I would actually question why you need such a property. The internal message stacks should be implementation details that clients should not need to access. They should be maintained by the network implementation. I see this property is currently being used to clear the stack by the main network class, but I question if that's good practice. This can be especially dangerous in multi-threaded environments.


public override void SendData()
{
    lock (AbstractNetworkSubSystem.sentMessagesLocker)
    {
        foreach (IMessage m in sentMessages)
        {
            //Add server code here
        }
     }
}

How does the server code fit in here? An explanation of how this will all work would be good.

I know it's nitpicking, but in the interest of clear code, receive is spelled with an ei, not ie.

I'll try to take a closer look at it soon, but these are my initial comments.
Jan 7, 2008 at 9:19 PM
Generally remember to use this prefix on instance members. Remember to add copyright notice at the top of the file

In MessageType.cs
  • On added consts add xml comments to all

In QSConstants.cs
  • On added consts add xml comments to all

In QSGame.cs
  • On Net do not shorts for words use full work in this case Network

In QuickStartSamleGame.cs
  • On Initialize do use this prefix for instance members

In AbstractNetworkSubSystem.cs
  • I can't see the added value of the extra namespace, for me it seems that everything belong under Metwork.
  • Add xml comments (everywhere!)
  • Do not expose fields, use properties
  • Do not use the name Abstract in names, it has a specific meaning in c#
  • Do use this prefix for instance members
  • On Update Thread.Sleep is a waste of CPU power never use it
  • Do not use the word SubSystem, it's confusing as to what it really means, ie LocalSubSystem of what, instead use LocalConnection or something similar
  • Why is State an enum it would be more efficient to use a static class with const for each, similar to MessageType

In LocalSubSystem.cs
  • On AvailableSessions will throw a stack overflow as it invokes itself
  • On SendCustomMessage you do not lock before pushing onto the stack which might raise a race condition.

In NetworkSystem.cs
  • Do use this prefix for instance members
  • Remember to use scope modifiers on all members even if they are private
  • On active do not expose fields, use properties. Also fields exposed should use PascalCase rather than camelCase
  • On Constructor do provide proper property names not just single letters
  • On Constructor do loose the event handler instantiation it's not needed
  • On IsSpecial I really do not like that name, every message is by it's own right special please rewise
  • On GameMessage do not lock on objects not belonging to the type you are currently assigning to. You can not be sure that subsystem is actually a AbstractNetworkSubSystem and might be locking on the wrong object.
  • On GameMessage you should check for null value
  • On InitializeNetwork rename this Initialize as it's obviously initializing the network, it's on the network class
  • On InitializeNetwork you do not check for not and will eventually throw a Null exception, please make sure your code is stable
  • On getAvailableSessions please rework as a property AvailableSessions (get methods are so C++)
  • On Update do not user single letters for variable names (with exception if i k etc for loops) use name, such as message, makes the code easier to undersand
  • On IsSpecial correct the indent as it's messed up
  • On IsSpecial consider only having one exit point for the method
  • GenericAvailableNetworkSession should be in it's own file.
Jan 8, 2008 at 3:48 PM
Edited Jan 8, 2008 at 4:48 PM

Looks like a great start so far!

Where are the comments? :) They really do make it a lot easier to understand code. A lot of times it's hard to guess exactly what a method is supposed to do with just the name and names of parameters. For instance, CustomUpdate and SendCustomMessage. What are their purpose?

Also, especially since you rely on threading, it would be good if each method comment contained a line about which thread calls the method, the main processing thread or a worker thread.


Cheers, glad its not all rubbish :)
Seriously, sorry about the comments. I either go overboard, or too few. I never seem to find the right balance. Ill make sure every method and public variable has the comments you aksed for.


Everywhere:

Why do you use extra objects for locking? i.e.

lock (AbstractNetworkSubSystem.currentStateLocker)
{
    subSystem.CurrentState = (int)State.FindingSession;
}

Is there a reason you're not locking on the object you're going to use, like subSystem.CurrentState?


When I began to do the threading for the system, I decided to brush up on my knowledge (especially thread safety) and this was recommended in an e-book I found. I suppose it makes no odds, as the variables themselves are suitable. That would make a small memory saving, and its clearer to ill swithc to that if you prefer.


LocalSubSystem.cs:

This is very dangerous:

public override Stack<IMessage> recievedMessages
{
    get { lock (AbstractNetworkSubSystem.recievedMessagesLocker) { return recievedMessagesStack; } }
    set { lock (AbstractNetworkSubSystem.recievedMessagesLocker) { this.recievedMessagesStack = value; } }
}

For the getter, you're only holding the lock for the retrieval of the stack instance. Once the client code gets the stack reference, the lock is no longer active. And for the setter, why not lock on recievedMessageStack. I would actually question why you need such a property. The internal message stacks should be implementation details that clients should not need to access. They should be maintained by the network implementation. I see this property is currently being used to clear the stack by the main network class, but I question if that's good practice. This can be especially dangerous in multi-threaded environments.


Ill remove it. I will leave it accessible as the main netowkr interface needs it. Instead ill make it internal, so only our code could access it. This should mean its always thread safe.


public override void SendData()
{
    lock (AbstractNetworkSubSystem.sentMessagesLocker)
    {
        foreach (IMessage m in sentMessages)
        {
            //Add server code here
        }
     }
}

How does the server code fit in here? An explanation of how this will all work would be good.

Im planning on getting the socket system done as soon as the current code is approved, to serve as a sort of example. The basic idea is the server (host) handles all messages recieved, and any messages destined for only the server are only sent there. This means I dont need to check, saving cou cycles. Its mainly up to the specific implementation though exactly how it will work.


I know it's nitpicking, but in the interest of clear code, receive is spelled with an ei, not ie.

I'll try to take a closer look at it soon, but these are my initial comments.


cue emarassed silence over spelling
Thanks for the comments there really useful, and much appreciated. Now onto Sturms, rather large, ominous list :P
Jan 8, 2008 at 3:57 PM

Sturm wrote:
Generally remember to use this prefix on instance members. Remember to add copyright notice at the top of the file

In MessageType.cs
  • On added consts add xml comments to all

In QSConstants.cs
  • On added consts add xml comments to all

In QSGame.cs
  • On Net do not shorts for words use full work in this case Network

In QuickStartSamleGame.cs
  • On Initialize do use this prefix for instance members

In AbstractNetworkSubSystem.cs
  • I can't see the added value of the extra namespace, for me it seems that everything belong under Metwork.
  • Add xml comments (everywhere!)
  • Do not expose fields, use properties
  • Do not use the name Abstract in names, it has a specific meaning in c#
  • Do use this prefix for instance members
  • On Update Thread.Sleep is a waste of CPU power never use it
  • Do not use the word SubSystem, it's confusing as to what it really means, ie LocalSubSystem of what, instead use LocalConnection or something similar
  • Why is State an enum it would be more efficient to use a static class with const for each, similar to MessageType

In LocalSubSystem.cs
  • On AvailableSessions will throw a stack overflow as it invokes itself
  • On SendCustomMessage you do not lock before pushing onto the stack which might raise a race condition.

In NetworkSystem.cs
  • Do use this prefix for instance members
  • Remember to use scope modifiers on all members even if they are private
  • On active do not expose fields, use properties. Also fields exposed should use PascalCase rather than camelCase
  • On Constructor do provide proper property names not just single letters
  • On Constructor do loose the event handler instantiation it's not needed
  • On IsSpecial I really do not like that name, every message is by it's own right special please rewise
  • On GameMessage do not lock on objects not belonging to the type you are currently assigning to. You can not be sure that subsystem is actually a AbstractNetworkSubSystem and might be locking on the wrong object.
  • On GameMessage you should check for null value
  • On InitializeNetwork rename this Initialize as it's obviously initializing the network, it's on the network class
  • On InitializeNetwork you do not check for not and will eventually throw a Null exception, please make sure your code is stable
  • On getAvailableSessions please rework as a property AvailableSessions (get methods are so C++)
  • On Update do not user single letters for variable names (with exception if i k etc for loops) use name, such as message, makes the code easier to undersand
  • On IsSpecial correct the indent as it's messed up
  • On IsSpecial consider only having one exit point for the method
  • GenericAvailableNetworkSession should be in it's own file.


Will do, your the style expert. You dont like c++ do you? :)

If there is anythin in patricular I disagree with Ill repost here to discuss it with you. Otherwise consider them all 'in progress'.
Jan 8, 2008 at 4:45 PM
Edited Jan 8, 2008 at 4:54 PM
  • Ive changed the lock code, so its now the object. That is except for the int, as its not a correct type. Thats still has a special lock object.

  • DOnt follow this, sorry Sturm:

In QuickStartSamleGame.cs

* On Initialize do use this prefix for instance members

Jan 8, 2008 at 6:21 PM
Using this prefix means that you should have "this." in front of instance members consider the following:
public class MyClass
{
    private bool enabled = false
 
    public void Initialize(bool enabled)
    {
        enabled = enabled;
    }
} 
This is obviously wrong and the assignement line should read:
        this.enabled = enabled;

If you always follow the guideline of using this prefix for instance members you wouldn't have that error in the first place. I know this is a very simple example, but trust me this is an issue in larger more complex projects using partial base classes, here it can be a nightmare finding these bugs as they might not be as evident. And the error might surface in a different place as well