`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](https://godbolt.org/#g:!((g:!((g:!((h:codeEditor,i:(filename:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,selection:(endColumn:42,endLineNumber:21,positionColumn:42,positionLineNumber:21,selectionStartColumn:42,selectionStartLineNumber:21,startColumn:42,startLineNumber:21),source:'%23include+%3Cmemory%3E%0A%0A%0A%0Astruct+Token+%7B%0A%0A++++++++~Token()%3B%0A++++++++friend+class+Storage%3B%0A++++private:%0A++++++++explicit+Token(int)%3B%0A%7D%3B%0A%0A%0Astruct+Storage+%7B%0A%0A++++virtual+std::unique_ptr%3CToken%3E+Attach()+noexcept+%3D+0%3B%0A%0A%7D%3B%0A%0Astruct+MyToken+:+Token+%7B%0A++++explicit+MyToken(int+x)+:+Token(x)+%7B%7D%0A%7D%3B%0A%0A%0Astruct+MyStorage+:+Storage+%7B%0A%0A++++std::unique_ptr%3CToken%3E+Attach()+noexcept+override+%7B%0A++++++++return+std::make_unique%3CMyToken%3E()%3B%0A++++%7D%3B%0A%0A%0A%7D%3B%0A'),l:'5',n:'1',o:'C%2B%2B+source+%231',t:'0')),k:45.01305483028721,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((g:!((h:compiler,i:(compiler:clang1600,filters:(b:'0',binary:'1',binaryObject:'1',commentOnly:'0',debugCalls:'1',demangle:'0',directives:'0',execute:'0',intel:'1',libraryCode:'0',trim:'0',verboseDemangling:'0'),flagsViewOpen:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,libs:!(),options:'-O3',overrides:!(),selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1),l:'5',n:'0',o:'+x86-64+clang+16.0.0+(Editor+%231)',t:'0')),header:(),k:50,l:'4',m:69.14556962025317,n:'0',o:'',s:0,t:'0'),(g:!((h:output,i:(compilerName:'x86-64+gcc+14.1',editorid:1,fontScale:14,fontUsePx:'0',j:1,wrap:'1'),l:'5',n:'0',o:'Output+of+x86-64+clang+16.0.0+(Compiler+%231)',t:'0')),header:(),l:'4',m:30.854430379746834,n:'0',o:'',s:0,t:'0')),k:54.98694516971278,l:'3',n:'0',o:'',t:'0')),l:'2',n:'0',o:'',t:'0')),version:4).

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](https://godbolt.org/#g:!((g:!((g:!((h:codeEditor,i:(filename:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,selection:(endColumn:42,endLineNumber:21,positionColumn:42,positionLineNumber:21,selectionStartColumn:42,selectionStartLineNumber:21,startColumn:42,startLineNumber:21),source:'%23include+%3Cmemory%3E%0A%0A%0A%0Astruct+Token+%7B%0A%0A++++++++~Token()%3B%0A++++++++friend+class+Storage%3B%0A++++private:%0A++++++++explicit+Token(int)%3B%0A%7D%3B%0A%0A%0Astruct+Storage+%7B%0A%0A++++virtual+std::unique_ptr%3CToken%3E+Attach()+noexcept+%3D+0%3B%0A%0A%7D%3B%0A%0Astruct+MyToken+:+Token+%7B%0A++++explicit+MyToken(int+x)+:+Token(x)+%7B%7D%0A%7D%3B%0A%0A%0Astruct+MyStorage+:+Storage+%7B%0A%0A++++std::unique_ptr%3CToken%3E+Attach()+noexcept+override+%7B%0A++++++++return+std::make_unique%3CMyToken%3E()%3B%0A++++%7D%3B%0A%0A%0A%7D%3B%0A'),l:'5',n:'1',o:'C%2B%2B+source+%231',t:'0')),k:45.01305483028721,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((g:!((h:compiler,i:(compiler:clang1600,filters:(b:'0',binary:'1',binaryObject:'1',commentOnly:'0',debugCalls:'1',demangle:'0',directives:'0',execute:'0',intel:'1',libraryCode:'0',trim:'0',verboseDemangling:'0'),flagsViewOpen:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,libs:!(),options:'-O3',overrides:!(),selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1),l:'5',n:'0',o:'+x86-64+clang+16.0.0+(Editor+%231)',t:'0')),header:(),k:50,l:'4',m:69.14556962025317,n:'0',o:'',s:0,t:'0'),(g:!((h:output,i:(compilerName:'x86-64+gcc+14.1',editorid:1,fontScale:14,fontUsePx:'0',j:1,wrap:'1'),l:'5',n:'0',o:'Output+of+x86-64+clang+16.0.0+(Compiler+%231)',t:'0')),header:(),l:'4',m:30.854430379746834,n:'0',o:'',s:0,t:'0')),k:54.98694516971278,l:'3',n:'0',o:'',t:'0')),l:'2',n:'0',o:'',t:'0')),version:4).

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[bot]]

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