afterincomparableyum commented on PR #3693: URL: https://github.com/apache/celeborn/pull/3693#issuecomment-4619438871
regarding CoPilot comments @SteNicholas, it is right that this posts a task per callback. But I don't think the use_count() == 1 guard is safe, so I'd prefer to keep the unconditional handoff. The whole point of the offload is that the reader's last reference must never be dropped on the fetch callback threaders it destroys the reader on the IO thread and triggers the EDEADLK issue this PR fixes. The problem with use_count() is that it's a stale snapshot. The reader's owner lives on another thread and can drop its reference at any moment (cleanupReader() is just currReader_ = nullptr). So: 1. Callback lifts shared_this --> refcount = 2 2. Checks use_count() --> sees 2 --> decides to drop inline 3. Owner thread drops its reference --> refcount = 1 4. Callback returns --> shared_this destructs on the IO thread --> same EDEADLK crash So the guard trades a correct path for a racy one whose failure mode is a process abort. With multiple chunks in flight, this interleaving is realistic. Regarding overhead. I will update this PR and add comments with a TODO to explore optimizations if needed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
