rados: Make sure Conn isn't GC'd while it has dependant IOContexts.#713
rados: Make sure Conn isn't GC'd while it has dependant IOContexts.#713mergify[bot] merged 1 commit intoceph:masterfrom
Conversation
|
After catching up on the other thread, I am largely in favor of just taking this change. It's simple, it's a very small delta between go-ceph as it is today, and it will fix issues people actually run into. It may not be the most rigorous option but I think it's the most practical. However, I won't approve it yet until I see buy in on this PR itself, as I don't want automerge to kick in just yet. |
|
I'm fine with merging this as is, because it's definitely an improvement to the current situation. But I would like to still have a conscious decision regarding the finalizer. I think it might bring us into the wrong direction. |
baergj
force-pushed
the
fix-conn-early-gc
branch
from
be646ae to
677088c
Compare
|
I've brought the PR up to date with |
baergj
force-pushed
the
fix-conn-early-gc
branch
from
677088c to
8775237
Compare
|
@baergj no need to force-push the rebased versions, else mergify will always remove the approvals. We need to use mergify for the rebase, as long as there are no merge conflicts. |
|
@ansiwen Good to know! Sorry, I haven't used mergify before. |
|
Hmm, I'm not seeing what failed in the Quincy tests (it just indicates that the rados tests failed, but not which one or why). Do either of you know how it failed? |
|
The mysterious, test stdout looks like a pass but job failed flake. Sorry, the other flakes seem to have causes but that one is unknown still. Regardless, I've restarted the job. It's unlikely to be related to the change. |
|
Looks like this is ready to merge - do you need anything more from my side? |
|
@Mergifyio rebase |
Because we have a finalizer configured for Conn that cleans up Ceph resources, it's important that we don't let it get GC'd before anything that depends on it has been cleaned up. Since clients are free to hold a reference to an IOContext without maintaining a reference to the underlying Conn, we hold such a Conn reference within the IOContext to signal this dependency to the Go GC. Signed-off-by: Joshua Baergen <jbaergen@digitalocean.com>
✅ Branch has been successfully rebased |
ktdreyer
force-pushed
the
fix-conn-early-gc
branch
from
8775237 to
cfba1d9
Compare
|
@baergj we should be good thanks! The mergify system just needs to see all merge criteria met at least once to add it to the merge queue. So it needs another rebase - after that I think it will accept it. |
phlogistonjohn
added
the
no-API
|
sigh it would help if I remember to put one of the required labels on :-) |
|
Thanks all! |
4 participants
Because we have a finalizer configured for Conn that cleans up Ceph resources, it's important that we don't let it get GC'd before anything that depends on it has been cleaned up. Since clients are free to hold a reference to an IOContext without maintaining a reference to the underlying Conn, we hold such a Conn reference within the IOContext to signal this dependency to the Go GC.
Fixes #569.