RuntimeContext::Attach returns a unique_ptr to a Token, but can not use a subclass

[maierlars]

I'm referring to runtime_context.h. The Attach method returns a Token but wrapped in a unique_ptr. At the same time the Token just contains a Context which is in fact just a shared_ptr to some data. I guess Token is supposed to be some kind of guard that ensures that a context is eventually detached.

Are my assumptions correct?

If so, then we don't need an allocation of a Token for that. Instead make Token a proper guard type, that semantically behaves like a unique_ptr. Is it possible to change that or am I missing something? Would this be considered a breaking api change?

I'm happy to provide a patch.

[marcalff]

To investigate. Not sure there is an issue with the current code, but this could be a posible cleanup or improvement.

[maierlars]

Thanks for the answer. I don't think there is a bug, its just convoluted. It's rather a suggestion for a refactoring/cleanup. I'm happy to provide a patch if you want to.

[maierlars]

I understand that everyone is busy. Is there anything I can do to make this issue more forward? Are you willing to consider pull requests with respect to this issue? Thank you very much for your time.

[marcalff]

Thanks for raising the issue.

The key point here is this comment:

/**
 * RuntimeContextStorage is used by RuntimeContext to store Context frames.
 *
 * Custom context management strategies can be implemented by deriving from
 * this class and passing an initialized RuntimeContextStorage object to
 * RuntimeContext::SetRuntimeContextStorage.
 */
class OPENTELEMETRY_EXPORT RuntimeContextStorage

About simplifying nostd::unique_ptr<Token> to a guard class:

• a guard class typically assumes that the calls to Attach() and Detach() are done from the same place, making the guard a useful helper for this case.
• there are cases when Attach() and Detach() are called from different places, when implementing a thread pool. A guard will be inconvenient for this.

About simplifying Token because it "only" contains a context:

• this makes the assumption that nostd::unique_ptr<Token> points to the exact class Token
• A subclass of RuntimeContextStorage, can return a subclass of Token, by using a custom token with more state.
• The token, for thread local storage (class ThreadLocalContextStorage), in indeed just a context, but this does not mean every token are the same.

Currently, the api is designed in such a way that:

• implementing a custom RuntimeContextStorage
• implementing a custom Token
• calling Attach and Detach from different places

is possible, which is likely to be the case for thread pools.

The proposed simplification will break this.

[maierlars]

Thanks for your detailed response. I appreciate it very much!

A subclass of RuntimeContextStorage, can return a subclass of Token, by using a custom token with more state.

Sadly, this is not possible as the code is right now, because the only constructor of Token is private and only RuntimeContextStorage is a friend of Token. The only way to construct a token is calling CreateToken on the RuntimeContextStorage. The Token class looks pretty much locked down. Probably I'm missing something. See godbolt.

there are cases when Attach() and Detach() are called from different places, when implementing a thread pool. A guard will be inconvenient for this.

Probably this is just an issue with naming things. For me a unique_ptr is very much a guard-like object. I don't see any difference in handling unique_ptr over any other move-only type.

[marcalff]

Thanks for your detailed response. I appreciate it very much!

A subclass of RuntimeContextStorage, can return a subclass of Token, by using a custom token with more state.

Sadly, this is not possible as the code is right now, because the only constructor of Token is private and only RuntimeContextStorage is a friend of Token. The only way to construct a token is calling CreateToken on the RuntimeContextStorage. The Token class looks pretty much locked down. Probably I'm missing something. See godbolt.

It looks like nobody has implemented a custom runtime storage with a custom token yet then.

This is a confirmed bug, the Token constructor should be protected or even better public.

A PR to fix this part will be welcome ;-)

there are cases when Attach() and Detach() are called from different places, when implementing a thread pool. A guard will be inconvenient for this.

Probably this is just an issue with naming things. For me a unique_ptr is very much a guard-like object. I don't see any difference in handling unique_ptr over any other move-only type.

Sure, a unique_ptr is very much a guard-like object.

However, changing this will break both the API and ABI.
Unless the current code is somehow flawed, and absolutely needs to be fixed, this can not change.

[github-actions]

This issue was marked as stale due to lack of activity.