Skip to content

Conversation

DenisBiryukov91
Copy link
Contributor

Resolves #986

Copy link

github-actions bot commented Apr 8, 2025

PR missing one of the required labels: {'documentation', 'dependencies', 'internal', 'enhancement', 'new feature', 'breaking-change', 'bug'}

@DenisBiryukov91 DenisBiryukov91 added the bug Something isn't working label Apr 8, 2025
@DenisBiryukov91 DenisBiryukov91 force-pushed the block-until-callback-drop branch from 4c55e9f to 882e5eb Compare April 8, 2025 17:55
@yellowhatter
Copy link
Contributor

If I correctly understand the code, it will deadlock if entity is dropped inside of the callback. If so - this is extremely complicated and also is a breaking change for existing code.

Can you please explain what the bug is? - as for me, if we just follow underlying zenoh logic (and we do), everything should be fine

@DenisBiryukov91
Copy link
Contributor Author

DenisBiryukov91 commented Apr 9, 2025

If I correctly understand the code, it will deadlock if entity is dropped inside of the callback. If so - this is extremely complicated and also is a breaking change for existing code.

I do not clearly understand how this could happen under normal circumstances.
I assume that we are going to drop entity inside its own callback's drop and not in callback's call (otherwise this is an UB anyways).

We can not pass subscriber/queryable/listener to their respective callbacks since they are not constructed yet - we could pass a non-shared pointer to the place where they would be constructed though - but this will result in UB if constructor/declare fails - since it is unclear what is going to happen earlier - the callback is dropped or we null the corresponding zenoh entity - to ensure that drop passes correctly, since we never explicitly request to pass nullified pointers.

Now we can pass session to a background subscriber/queryable declaration - I assume that if we do - we must do it using shared pointers - at least this is how we are forced to do in rust - otherwise this is again a way to UB. In this case callback will always hold a reference to session which means - it will never be dropped, since we get a circular dependency session<->callback. The only way to break this circular dependency is by calling session.close() - which is fine here since it will first destroy callbacks, holding shared ptrs to the session, without calling session's destructor.

The last case is to pass session's shared ptr to a get callback. In this case callback's drop will be triggered in session.send_response_final - but this already happens while we hold a session.state lock (which is required for session drop/close). So it will deadlock anyways - with my pr or without .

Can you please explain what the bug is? - as for me, if we just follow underlying zenoh logic (and we do), everything should be fine

Zenoh logic does not provide any guarantee regarding when the callback will be called the last time nor when it's drop will be called. This creates issues namely in C++ when subscriber and callback data are both stored in the same object - since very often it results in invoking callback or its drop after related data is destroyed - this obviously leads to difficult to debug situation which has been already reported by our users multiple times.

@DenisBiryukov91 DenisBiryukov91 force-pushed the block-until-callback-drop branch from e0afff8 to ae86737 Compare September 25, 2025 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Ensure that callbacks are never called when subscriber/queryable/session is dropped
3 participants