Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update enet_packet_destroy documentation to reflect usage #127

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bowbahdoe
Copy link

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.

… used.

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.
@bjorn
Copy link

bjorn commented May 25, 2020

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. :-)

@bowbahdoe
Copy link
Author

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?

@SimonN
Copy link

SimonN commented Feb 20, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants