[EXPORTER] Fix OTLP HTTP exporting in sync mode.

[owent]

Fixes #2191

Changes

Please provide a brief description of the changes here.

• Use shared_ptr to hold the lifetime when using OTLP HTTP exporting in sync mode.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[codecov]

Codecov Report

Merging #2193 (c1b03d2) into main (4b20067) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2193   +/-   ##
=======================================
  Coverage   87.52%   87.52%           
=======================================
  Files         169      169           
  Lines        4887     4887           
=======================================
  Hits         4277     4277           
  Misses        610      610           

[marcalff]

Thanks for the fix.

From the bug report,

the lambda capture [&session_result] causes a capture of a reference to a local variable, causing the bug.

From the fix,

the lambda capture [session_result] now captures a shared pointer, presumably incrementing the reference counter upon capture.

From existing code,

the callback function in OtlpHttpClient::Export can be given to a session, with

auto session = createSession(message, std::move(result_callback));

and then the session, which owns the callback, is stored in running_sessions_ by:

addSession(std::move(opentelemetry::nostd::get<HttpSessionData>(session)));

So, when some code paths return from ::Export() on timeout, the callback is kept elsewhere already, and local variables (in the old code) are destroyed.

Please:

1. Confirm my understanding is correct

2. Detail where / when is the destructor called for the lambda capture [session_result], that would decrement the last reference to the session_result, and destroy the object created by make_shared in the fix ... I don't understand this part.

[owent]

Thanks for the fix.

From the bug report,

the lambda capture [&session_result] causes a capture of a reference to a local variable, causing the bug.

From the fix,

the lambda capture [session_result] now captures a shared pointer, presumably incrementing the reference counter upon capture.

From existing code,

the callback function in OtlpHttpClient::Export can be given to a session, with

auto session = createSession(message, std::move(result_callback));

and then the session, which owns the callback, is stored in running_sessions_ by:

addSession(std::move(opentelemetry::nostd::get<HttpSessionData>(session)));

So, when some code paths return from ::Export() on timeout, the callback is kept elsewhere already, and local variables (in the old code) are destroyed.

Please:

1. Confirm my understanding is correct

2. Detail where / when is the destructor called for the lambda capture `[session_result]`, that would decrement the last reference to the session_result, and destroy the object created by make_shared in the fix ... I don't understand this part.

1. Yes, that's right.
2. The callback is stored in HttpSessionData which is holded by OtlpHttpClient::running_sessions_, OtlpHttpClient::gc_sessions_ .We remove session data from running_sessions_ only when we get a event or response from http client.And when wait for response in sync mode, session_waker_.wait_for may be timeout before we get any event from it.

[marcalff]

LGTM, thanks for the fix and clarifications.

[lalitb]

Looks good. Thanks for the fix. This part of code has lots of edge scenarios, would be good to extend the unit-tests to cover them sometime later.

[marcalff]

/easycla

[ThomsonTan]

Seems this current fix requires a heap allocation, even this is sync Export. And the heap allocation is only for the edge case (timeout). Wondering is it possible to avoid the heap allocation on ExportResult here?

[owent]

Seems this current fix requires a heap allocation, even this is sync Export. And the heap allocation is only for the edge case (timeout). Wondering is it possible to avoid the heap allocation on ExportResult here?

By now, I have no idea without break the behaviour of return value.