Issue in multi-thread with ENetEvent?

[gregoire-astruc]

Per the documentation:

as long as the application guards access to [ENetHost], then ENet should operate fine in a multi-threaded environment.

The problem is accessing the peer field of ENetEvent actually boils down to accessing ENetHost.

Consider this:

Thread 1

while(enet_host_service(host, &event, ...) >= 0)
{
   events.enqueue(event);
}

Thread 2

auto event = events.dequeue();
/* process event... */
enet_peer_send(event.peer, ...);

Now, under contention on the peers, it can happen that Thread 2 will not send to the right peer.

By the time Thread 2 calls enet_peer_send, enet_host_service may have recycled the peer for a different client, and Thread 2 will be none the wiser and happily send to the wrongful client.

Now maybe this is a wrongful usage, but the docs never warn about it.

Maybe I missed something in the library that handles however :)

[mattdinthehouse]

The ENetEvent structure you pass to enet_host_service() is "where event details will be placed" so every time Thread 1 calls enet_host_service() it gets overwritten. If you're enqueue()-ing a pointer to that ENetEvent then naturally the memory at that address will get overwritten and Thread 2 will get corrupt data. You should enqueue() a copy of it instead.

This isn't a bug in enet or its documentation and more just how threading works.

[gregoire-astruc]

The issue is not a stale event, but a stale or different peer for an event: the peer field inside ENetEvent points directly to ENetHost.

Therefore it is not as self contained as the documentation would lead you to believe, and accesses to event.peer should be protected given they are indeed accesses to ENetHost under the cover.

[bjorn]

I don't personally think the documentation is misleading... at least to me it was clear that the host owns its peers and that as a result you can't have some other thread directly access them without locking on the host object. Instead of locking, the easiest thing to do is usually to make sure to only use each host from a single thread.

In your example you have in addition the problem of the ENetPacket lifetime, which is probably managed by the host as well. The packet pointer in the ENetEvent is not guaranteed to stay valid after the next call to enet_host_service. Just consider the alternative: if it was guaranteed to stay valid then you would have to clean it up somewhere.