enet icon indicating copy to clipboard operation
enet copied to clipboard

Update enet_packet_destroy documentation to reflect usage

Open bowbahdoe opened this issue 5 years ago • 3 comments

I was working on a project and kept getting a segfault until i read deeper into the docs to know that enet_packet_send takes ownership of the packet. It might be beneficial to document that nearer the destroy function, but honestly I'm not sure the best way to clarify.

bowbahdoe avatar May 25 '20 20:05 bowbahdoe

The tutorial mentions:

Once the packet is handed over to ENet with enet_peer_send(), ENet will handle its deallocation and enet_packet_destroy() should not be used upon it.

It is somewhat misleading to say that enet_peer_send handles the destruction, since it merely queues the packet for sending (which is also why it can't already be destroyed).

I do agree that it may be helpful to make a note about this in general. Though, people could read the tutorial as well before making up their own usage pattern. :-)

bjorn avatar May 25 '20 20:05 bjorn

It is mentioned, but at least in my case i was working through a codebase that was messy in other ways and did leak memory as time went on, so I was looking for unfreed memory already. enet_peer_send also doesn't really address that it takes ownership of the packet in its documentation.

Maybe the right thing would be to just add a comment to the functions that take ownership of the packet's memory?

bowbahdoe avatar May 25 '20 21:05 bowbahdoe

Current behavior in ENet 1.3.17 is:

  • If enet_peer_send succeeds (returned 0), it has taken ownership of the ENetPacket that usercode passed. Usercode should not deallocate the packet afterwards, or do anything else with it.
  • If enet_peer_send fails (returned something < 0), it has not taken ownership of the packet. Usercode still owns the packet. Usercode should manually deallocate now with enet_packet_destroy, or perhaps wait and re-send the same ENetPacket later.

There are several possible routes forward:

  • Explain in the doxygen that enet_peer_send owns the packet if and only if it returned 0, and suggest that usercode, accordingly, (not) call enet_packet_destroy on that packet. This is what this PR aims for, at least for the success case.
  • Amend the tutorial: Its example code should check the return value and deallocate the packet on failure. The nearby paragraph that explains enet_peer_send's deallocation behavior should also say when it deallocates.
  • Change the implementation of enet_peer_send to always assume ownership, even on failure to send. But this is a breaking change: I've written a wrapper around enet_peer_send that deallocates the packet on failure. Such wrappers would break.

What's best? I'm happy to write my own PR for the docs.

I believe that library tutorials should, in general, be rock-solid production-ready code that people should be able to readily copy-and-paste. For the record, eihrul disagreed in IRC:

<SimonN> Where should I send a pull request for the tutorial?
    Tutorial code should ideally be production-ready,
    people will happily copy & paste.
<eihrul__> it's not meant to be copy and pasted
<SimonN> In practice, people will do it though.
<eihrul__> the goal is still to be a tutorial
    and not to be a complicated example of a real world application

SimonN avatar Feb 20 '22 19:02 SimonN