Fix missing notify of conditional variable in OTLP HTTP exporter test

[ThomsonTan]

Fixes #857

Changes

Added notify_one call to cv_got_events right after HTTP response processing.

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 #858 (066484a) into main (7fca2c3) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #858      +/-   ##
==========================================
+ Coverage   95.49%   95.50%   +0.02%     
==========================================
  Files         156      156              
  Lines        6618     6620       +2     
==========================================
+ Hits         6319     6322       +3     
+ Misses        299      298       -1     

Impacted Files	Coverage Δ
ext/test/http/curl_http_test.cc	95.22% <100.00%> (+0.59%)	⬆️

[lalitb]

why is this needed ? cv.wait_for is supposed to be unblocked with a timeout. I think this will fail now if the test is modified so that cv.wait_for has multiple requests to wait for, as now it will be unblocked after a single request?

[ThomsonTan]

Timeout in cv.wait_for is the last resort. cv should be waked up anytime its checking condition could be changed, so its condition could be re-checked instead of waiting for timing out. notify_one just tell cv to recheck so it should not trigger failure behavior?

cc @owent

[lalitb]

Does it mean that without notification, the cv will keep blocked for 2sec (as configured) even though the condition is satisfied early?

[ThomsonTan]

Yes, this is the case the PR trying to fix.

[lalitb]

Asking because we are using this construct in other places too:

opentelemetry-cpp/ext/test/http/curl_http_test.cc

Line 124 in 7fca2c3

if (cv_got_events.wait_for(lk, std::chrono::milliseconds(1000 * timeOutSec),

, probably they need a change too ( in separate PR ).

[ThomsonTan]

You are right, it is needed in curl_http_test.cc as well. Added the change to the PR and avoid another 2 timeout to happen there. Thanks for the catch.

[owent]

Timeout in cv.wait_for is the last resort. cv should be waked up anytime its checking condition could be changed, so its condition could be re-checked instead of waiting for timing out. notify_one just tell cv to recheck so it should not trigger failure behavior?

cc @owent

You are right. LGTM.

[owent]

Could you please remove the TODO I omit in #853 ? I think there is no need to create a PR just to remove the TODO comment.

[ThomsonTan]

Could you please remove the TODO I omit in #853 ? I think there is no need to create a PR just to remove the TODO comment.

Sure, just removed that.