[ENet-discuss] enet comments

[Lee Salzman]

On Fri, Mar 07, 2003 at 01:42:33PM -0800, Brian Hook wrote:
> Hi everyone,
> 
> The first minor point is that enet defines its own uint32, etc.  
> There is a potential for a name clash here with other packages.  I've
> recommended using posh.h (www.poshlib.org) to help streamline this
> and make it portable to a larger variety of platforms.
> 
    The typedefs I've already fixed in the latest source tree. However,
as for poshlib, ENet is a relatively stand-alone library. It has no
external dependencies whatesoever. I'd almost rather keep it that way.

> The second minor point is that the way the files are named, e.g.
> win32.c, can cause some conflicts with other files named win32.c.  
> Specifically, for people that have build systems that dump object
> files into a repository like /tmp/o, you can get a win32.c that
> clobbers another win32.c.  While this can be fixed by building enet
> as a library, some people prefer monolithic build systems for
> whatever reason, and it would be trivial to rename the files as
> en_win32.c, etc.  
> 
    I'm still not entirely sold on this point. I think that's a pretty
sloppy development practice, and most libraries have specific build
configuration and systems that should ideally be used to properly build
the variable. If someone wishes to create all manners of insane build
problems by not simply building a static library as the build system
for the library prescribes, that never has to be rebuilt or relinked, 
then I'm not sure they're sane enough to properly use the library. :)

> The third minor point is that enet requires enet/include in the
> global header search path, which is another minor but aggravating
> point when trying to integrate open source.  It would be nice if that
> was restructured so that everything just sat in a single directory
> and enet could be included via #include "enet.h" and the application
> could include it with an absolute or relative path as required.
> 
    Similarly to the problem you describe with clashing object files,
include files can clash as well. Ensuring the include files are properly
isolated in a directory isolates this problem. If nothing else,
"enet/enet.h" would be better than "enet.h" for usage within the source
code of ENet itself, so someone else's "win32.h" doesn't get accidently
included.
    
> The above come together on "how do I use this stuff?"  The sign of a
> popular and easy to use open source library is when you can just open
> up a directory and add all the files to a project or makefile, and it
> just compiles and builds.  lua, ijg, ogg vorbis, zlib, etc. all
> behave this way to some extent.
> 
    As I described above, I think is is a REALLY sloppy and abusive
practice that really shouldn't work at all and really shouldn't be
catered to. I don't mean to be harsh on this point, but I just don't 
see why I should allow for this.

> Those are minor points, but I thought I'd at least mention them.
> 
> Slightly more major points follow.
> 
> First, enet calls malloc()/free() directly instead of allowing the
> user to specify the memory management routine.  Some open source
> libraries allow the user to override these and similar routines by
> user defined callbacks.
> 
> Even with a user-specified malloc()/free(), I'm concerned that it may
> be open to fragmenting the user's heap.  Compaction and minimizing
> fragmentation are extremely important when dealing with large numbers
> of clients.  My own library keeps a ring buffer per client/peer for
> resends, and each client has its own separate buffer.  This is
> somewhat wasteful (vs. a global pool), but it's a lot more
> predictable and a single bad client won't affect the entire pool.  
> Fragmentation is a non-concern for my own stuff.
> 
    I have no problems with allowing user-specifiable allocation
and deallocation callbacks. However, I don't think ENet needs to go
through great pains internally to manage reuse. A good malloc, and
even mediocre ones, will pool like-sized objects together and try
to reuse from these pools as often as possible. I've done extensive
profiling with ENet to weed out any particular slow spots, and
even with huge volumes of packets, very little time is spent
on allocation and deallocation with the platform malloc()/free().
The majority of time is wasted in the actual sockets calls. I think
most people underestimate the efficiency of modern malloc
implementations.

> Second, I'm not sure if enet is re-entrant and/or thread safe.  My
> instinct is that it is not since there is no context to pass around,
> but I haven't looked at the code closely enough to see if this is an
> issue or not.
> 
    Pretty much all the context is stored in the ENetHost structure.
There's not really any major source of non-reentrancy or things that
prevent thread-safety as-is. The only global variable I have is to cache
the current time, as I found via the profiler that repeated invocations 
of gettimeofday() were, ironically, taking up unjustifiable amounts of 
time, presumably because GNU libc does not optimize this syscall.
The worst that can happen is some other host being polled in another
thread sets this variable, which won't have any adverse effect on
another polling host. ENet should be entirely reentrant and thread-safe.

> Third, I'm unclear how well enet can scale up to hundreds or
> thousands of clients.  The technological hurdles for each order of
> magnitude change, and enet obviously works on the lower end, but it
> might be nice (although maybe outside its scope) to ensure that it
> doesn't fall apart with 1000 clients.  I haven't looked enough at its
> implementation to really judge.
> 
    The only only real barrier to this is how ENet does iterates over
all clients looking for packets to send out and dispatch in some cases.
So, as we had discussed earlier, I may just add in either a single active
peer list or a few separate queues for peers with packets to send or
dispatch.

> Fourth, I would highly recommend making a quick overview document
> that describes the API, a quick start tutorial, and then some
> technical discussion to address some of the questions I've brought up
> and other issues, such as working with NATs, etc.
> 
    Did this today already too.

> Okay, that covers that.  
> 
> Now I'll talk about my own networking library and how I've done
> stuff, mostly to spark discussion for architecting this type of
> thing.  I don't think I've done things the right way, I've just done
> them the most intuitive way to me.  In addition, my stuff isn't
> battle tested, so a lot of this is theoretical.
> 
> Instead of treating everyone like peers, my library has the concept
> of "listen sockets" and "send sockets".  Clients are an abstraction
> used to validate incoming packets.
> 
> The general structure is still connectionless.  A server grabs a
> packet, and if it's a login packet (server specified) then it
> validates and possibly adds that client to its authorized client
> list.
> 
    In ENet, there's a certain amount of bidirectionality implied
in even the handshakes that take place to implement various protocol
commands. So there was no reason to not make connections bidirectional,
since I had to implement it for the protocol. I also think that the
bidirectional connections, with multiple packet channels, just offers
a simpler interface.

> One of the design goals of my library was a bad idea, in hindsight,
> and that was to support being a raw abstraction on sockets -- I
> wanted people to be able to use it to write a client that works with
> someone else's protocol.  I think this added needless complexity to
> my package, so I may eschew that.  The biggest problem is that I was
> computing CRCs as a way of flagging that it was a compatible packet
> type, since it had no other way of telling (magic cookies could end
> up with spurious, but eventually ignored, packets).
> 
    I agree with you there. Although ENet does provide a little raw socket
layer, it's just an artifact of the implementation, but can still
be exploited for use where expedient.

> The general system works as follows (for reliable data).
> 
> Each client connects to the server.  The client has a client
> identifier (based on a hash of its two low order bytes in IP) which
> is used to differentiate between clients behind a NAT.
> 
> The header is examined, and then a look up is performed in a hash
> table of clients.  A linear search search is then performed at that
> entry, matching IP against the incoming IP, and then finally matching
> client ID vs. client ID.
> 
> The most recent valid packet's address is then stored off so that I
> have a valid IP/port combination for sends.  I do NOT use the
> client's IP + port at connect time, since this can change.  Also, the
> server does not have to send to a specified port on the client, since
> with NAT this would require forwarding to each machine, which is a
> hassle for many users.  Doing a sendto() on the most recent
> recvfrom() address is a bit of a pain, but at least solves the
> firewall headache (although excessively paranoid firewalls, like
> corporate ones, might still be a concern).
> 
    I appreciated the input on this, as it was pretty simple to fix in
ENet once I knew the problem existed. :)

> Currently the app is responsible for detecting zombie connections or
> overflow conditions and manually removing a client in that situation.
> I wanted the app to manage connections because there are various DoS
> attacks that can be people can do depending on the network
> implementation.
> 
    The problem with that is then every person needs to be an expert
on DoS attacks. I think it's better to encapsulate this expertise in
the libray itself, and just let the developer write games.

> For example, if a protocol sends a "connect packet" and then the
> server allocates space for that client without a reasonable time
> frame to dispose of that client, then you can easily flood a server
> with multiple connect packets and varying session/client id values.  
> In a manner of minutes a server could think it has several thousand
> clients attempting to connect, causing overflow conditions or just
> really bad performance.
> 
    Currently this can be exploited ENet, in so far as there is a
maximum limit on clients. You could spam a lot of connect packets
and fill up all the connection slots. However, other than preventing
new connections for a few minutes, it shouldn't harm anything currently.

    Separating potential connections from established connections would
mostly solve this problem in ENet. However, then you need to limit
the number of potential connections, or else somebody could cause a
flood of potential conncetions structures to be allocated, and still
crash the server.

    So one way would be to limit the number of potential connections,
but rather than dropping new potential connections when the limit is
reached, you drop old potential connections, and allow the new one to
succeed. Even in the middle of an attack, this would allow actual
clients to connect. Combine with just preventing obscene numbers of 
reconnects within a certain time frame from a given host, and you could 
effectively prevent this attack.

    Alternatively, you could just employ this scheme on the connection
structures themselves, and not bother with distinguishing between
potential and established connections.

    I'll have to think about this a bit more ENet, though.

> Originally I was going to support having encryption/per-packet
> compression hooks, but I've decided that's really outside its domain,
> so I will make that an app thing.
> 
    Yep, I think this is simple enough and just better to do above
the protocol layer. I don't think there's a pressing reason to do it
below.

> My "utility" library provides services like delta
> compression/decompression, which I think can be generally useful for
> a lot of people, but isn't exactly 100% necessary by any means
> either.
> 
    Delta compression has always intrigued me in that a lot of things
seem to use it, but I've never been sure on what kind of potential space
savings it would really offer. Can't redundancy in the protocol mostly
be solved by a better protocol? Any more info in that regard?

> Brian
> 

    Lee