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

Jump to bottom

Another small memory leak in network code #846

Closed
DorpsGek opened this issue Jun 9, 2007 · 3 comments
Closed

Another small memory leak in network code #846

DorpsGek opened this issue Jun 9, 2007 · 3 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)

Comments

@DorpsGek
Copy link
Member

DorpsGek commented Jun 9, 2007

benc opened the ticket and wrote:

Semi-related to #844, only this time on the client side. :) In network_data.cpp : NetworkSend_Command, a CommandPacket is malloc'd for use in the server's local cmd queue. However if it's the client, the CommandPacket* is allowed to go out of scope without being free'd.

So every time a client sends a cmd to the server, he leaks a little over 100 bytes of memory.

The quick fix is easy, just free it. A possibly better solution is to not put the CommandPacket on the heap to begin with. The attached patch eliminates the malloc unless it's truly needed.

Attachments

Reported version: 0.5.2
Operating system: All

This issue was imported from FlySpray: https://bugs.openttd.org/task/846
@DorpsGek
Copy link
Member Author

Rubidium wrote:

I see the problem and the reason why the malloc isn't needed, but:
- there is absolutely no reason to make the command packet static (in global memory), allocation on the stack is good enough
- the logic of the function (even the whole network protocol) changes due to your changes that "look like" an optimization, but are not; clients get the callback too which they shouldn't get.

Pointing out these issues is great though.

This comment was imported from FlySpray: https://bugs.openttd.org/task/846#comment1307

@DorpsGek
Copy link
Member Author

benc wrote:

Yeah, using static was overkill, sorry about that. :)

I could have sworn I had set c.callback back to 0 before sending to clients. Wasn't trying to optimize so much as reorganize the function and make it more readable.

I noticed a fix isn't in trunk yet... in case it would be useful, a better patch (using stack and no logic changes) is attached. Thanks, and I'll continue to keep an eye out.

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/846#comment1321

@DorpsGek
Copy link
Member Author

Rubidium closed the ticket.

Reason for closing: Fixed

In r10082. I really like bugreports with fixes attached :). Thank you very much.

This comment was imported from FlySpray: https://bugs.openttd.org/task/846

@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) labels Apr 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@DorpsGek