Memory leak when using Rpc server

[pgScorpio]

Describe the bug
CServerRpc and CClientRpc/CServerRpc are created with "new" in main.cpp, but are never deleted.

To Reproduce
Run Jamulus with and without valid --jsonrpcport and --jsonrpcsecretfile parameters.
Use Visual studio's memory usage diagnostic tool to check the difference in memory usage at exit.

Expected behavior
Cleanup on exit.

Screenshots

Operating system
n.a.

Version of Jamulus
master @ 953f2b8

Additional context

[hoffie]

Thanks for the report.
Without having looked at the code, these seem to be the handler instances, so they are only instantiated once on startup, right?

I vaguely remember that other init-once stuff was not properly cleaned up on exit in the past, but I'm not totally sure and don't remember if it might have been fixed meanwhile.

I think it makes sense to fix it in order to have a clean output when chasing more relevant leaks.

Are you planning to submit a PR for this issue? If so, that would be great and I'll assign it to you. It will hopefully be a non-controversial PR which can be merged quickly.

(I've edited the description to point to a master commit as 3.8.2 (final) can't have been affected as it didn't have the RPC code yet)

[dtinth]

@pgScorpio Hello, thanks for the report on memory leaking issue. I'm the original author of the JSON-RPC code.

IMO this issue may not be a high impact one because the RPC server is initialized once and used throughout the lifetime of the process. Still I agree that it is a memory leak nonetheless and it is better to have a cleaner output from memory profiler. Most of my programming background is in a garbage-collected language and so I forgot how to properly manage memory in C++ already 😂

On my first implementation, I allocated an RPC server directly on the stack, but after code reviews, it has been changed to use C++ smart pointers, and subsequently to use just the new keyword. I'm not sure how to properly deallocate objects when it went out of scope (similar to Go's defer) without using smart pointers, so if you can contribute a proper fix, I would appreciate it. 😄

[pgScorpio]

@hoffie

I vaguely remember that other init-once stuff was not properly cleaned up on exit in the past, but I'm not totally sure and don't remember if it might have been fixed meanwhile.

Yes, there is still a lot more. i.e. ASIOSDK also has a big memory leak, and we can't change that, but that should be solved with the new Sound implementation (not using those ASIOSDK classes/functions.)

I think it makes sense to fix it in order to have a clean output when chasing more relevant leaks.

Are you planning to submit a PR for this issue? If so, that would be great and I'll assign it to you. It will hopefully be a non-controversial PR which can be merged quickly.

Yes, I just did a pull request, See #2618

[pgScorpio]

@dtinth

@pgScorpio Hello, thanks for the report on memory leaking issue. I'm the original author of the JSON-RPC code.

IMO this issue may not be a high impact one because the RPC server is initialized once and used throughout the lifetime of the process. Still I agree that it is a memory leak nonetheless and it is better to have a cleaner output from memory profiler. Most of my programming background is in a garbage-collected language and so I forgot how to properly manage memory in C++ already 😂

Unfortunately C++ has no garbage collections and a memory leak is easily created....

On my first implementation, I allocated an RPC server directly on the stack, but after code reviews, it has been changed to use C++ smart pointers, and subsequently to use just the new keyword.

"using C++ smart pointers" ?? for ServerRpc and ClientRpc there wheren't pointers at all, just new...

I'm not sure how to properly deallocate objects when it went out of scope (similar to Go's defer) without using smart pointers, so if you can contribute a proper fix, I would appreciate it. 😄

Well deallocating objects when the went out of scope can only be done via a class where the destructor deallocates (And a smart pointer is such a class.)

The problem here is that they are created conditionally and so do not have a specific scope...

[pljones]

Moved to 3.9.1

[pljones]

Moving to 3.10.0.