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

Fix payload memory leak #22

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

Conversation

mikezackles
Copy link

Previously, the sig_base destructor did not delete conn_base pointers if they
were owned by a connection, but it did null their back references to the signal,
which prevented them from destroying their payloads, which are placement-newed
inside a vector owned by the sig_base instance. Thus payloads were leaked.

This commit introduces an explicit destroy_payload virtual function to conn_base
so that the sig_base destructor can ensure that payloads are destroyed and still
allow the owning connection to destroy the conn_base pointer.

This also fixes what seemed like a bug with conn_nontrivial destruction not
handling blocked signals properly.

Previously, the sig_base destructor did not delete conn_base pointers if they
were owned by a connection, but it did null their back references to the signal,
which prevented them from destroying their payloads, which are placement-newed
inside a vector owned by the sig_base instance. Thus payloads were leaked.

This commit introduces an explicit destroy_payload virtual function to conn_base
so that the sig_base destructor can ensure that payloads are destroyed and still
allow the owning connection to destroy the conn_base pointer.

This also fixes what seemed like a bug with conn_nontrivial destruction not
handling blocked signals properly.
@dumblob
Copy link

dumblob commented May 31, 2021

@mikezackles do you happen to know why @TheWisp is inactive for about a year already? I'm worried a bit...

@mikezackles
Copy link
Author

No, I don't know why. If there is interest, I might be willing to consider maintaining a fork that includes my PRs, but I don't have a ton of time to commit.

@dumblob
Copy link

dumblob commented May 31, 2021

Ok. I'd say let's wait a few weeks and if @TheWisp won't show up, make a new issue in the issue tracker with the "development happens now in ... repository".

Do you have commit rights in this repository of @TheWisp ? If so, you could then lock all discussions to avoid mess during the "transition".

@mikezackles
Copy link
Author

Do you have commit rights in this repository of @TheWisp ?

No, I do not have commit rights.

Ok. I'd say let's wait a few weeks and if @TheWisp won't show up, make a new issue in the issue tracker with the "development happens now in ... repository".

Hmm, I'm pretty hesitant to make that type of announcement without hearing from the author. But perhaps at some point it would be OK to mention that there is an alternative fork if this project remains dormant.

My thought was just that I intend to continue using these changes anyway, so I'll be maintaining them one way or another (maybe just within my own projects).

But yes, let's give it some time for thought, and in the meantime it would be nice to hear from @TheWisp. Personally I'm appreciative of their work on this project and just wondering about what the best path forward is.

@TheWisp
Copy link
Owner

TheWisp commented Jun 2, 2021

Hi @dumblob and @mikezackles , thanks for caring about the project and my safety. A lot have happened last year - I moved country and change job to join one of the top software companies. I promise I'll take a look at it this weekend!!!

@dumblob
Copy link

dumblob commented Jun 2, 2021

Congratulations @TheWisp! And thanks for chiming in here!

Speaking of bus factor, have you considered giving few other devs enough permissions to this repository (or creating a separate GitHub organization for this purpose) to allow repo archiving, committing, and discussion locking?

@mikezackles
Copy link
Author

Yes, congratulations -- that's fantastic! Life is more important, so please don't feel rushed.

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.

None yet

3 participants