[BUG] Publish the completion callback before the first event

Cleanup() takes is_cleaned_ at its start and swaps async_data_->callback about
forty lines later, so the is_cleaned_ recheck in SendAsync only says cleanup
has begun, not that it has finished reading the callback.

A handler cancelling from the Connecting event wakes the IO thread, which can
reach the swap before SendAsync stores the callback. Cleanup then finds an
empty one, the completion never runs, and the lambda that clears
is_session_active_ never runs with it, so the session stays active for good.
The two accesses to the std::function also race.

The callback is published with the flags and the session route now, before the
event. The future stays behind it, so a handler calling FinishSession() from
that event still returns rather than waiting on a transfer that has not been
scheduled.

Found by @lalitb in review.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

Author: thc1006

ext/src/http/client/curl/http_operation_curl.cc

@@ -1452,20 +1452,24 @@ CURLcode HttpOperation::SendAsync(Session *session, std::function<void(HttpOpera
   }
   curl_easy_setopt(curl_resource_.easy_handle, CURLOPT_PRIVATE, session);
 
-  // A handler can cancel from this event, so the flags and the route it needs are published
-  // before it runs. They used to be written afterwards, which threw such a cancel away.
+  // A handler can cancel from this event, so everything Cleanup() needs has to be published
+  // before it runs. The callback most of all: Cleanup() takes is_cleaned_ at its start and
+  // swaps the callback much later, so a cleanup racing an assignment placed after the event
+  // would swap an empty one, never run the completion, and leave the session marked active
+  // for good.
   is_finished_.store(false, std::memory_order_release);
   is_aborted_.store(false, std::memory_order_release);
   is_cleaned_.store(false, std::memory_order_release);
-  async_data_->session = session;
+  async_data_->session  = session;
+  async_data_->callback = std::move(callback);
 
   DispatchEvent(opentelemetry::ext::http::client::SessionState::Connecting);
 
-  // The future stays unpublished until after the event, so a handler that calls FinishSession()
-  // from it returns instead of waiting on a transfer that has not been scheduled yet.
+  // The future is the one thing that stays unpublished until after the event, so a handler that
+  // calls FinishSession() from it returns instead of waiting on a transfer that has not been
+  // scheduled yet.
   async_data_->result_promise = std::promise<CURLcode>();
   async_data_->result_future  = async_data_->result_promise.get_future();
-  async_data_->callback       = std::move(callback);
   async_data_->is_promise_running.store(true, std::memory_order_release);
 
   // Cancelling from the event hands the session to the IO thread, which may already have torn

ext/test/http/curl_http_test.cc

@@ -593,10 +593,10 @@ TEST_F(BasicCurlHttpTests, RetryJitterIsNotSharedAcrossThreads)
   drawing_second.join();
 }
 
-// A handler is allowed to cancel from the events SendRequest dispatches. Those used to be
-// dispatched before the flags and the cancel route were published, so the cancel was thrown
-// away and the request was neither sent nor completed, leaving FinishSession() waiting on a
-// promise nobody could fulfil. See #4390.
+// A handler is allowed to cancel from the events SendRequest dispatches, so the flags and the
+// cancel route have to be published before the dispatch. Publish them after it and the cancel
+// is thrown away, the request is neither sent nor completed, and FinishSession() waits on a
+// promise nobody can fulfil. See #4390.
 TEST_F(BasicCurlHttpTests, CancelFromCreatedCompletes)
 {
   auto session_manager = std::make_shared<http_client::curl::HttpCurlClientFactory>()->Create();
@@ -640,8 +640,8 @@ TEST_F(BasicCurlHttpTests, CancelFromConnectingCompletes)
 }
 
 // CreateSession hands back an unregistered session when the URL does not parse, and
-// CURLOPT_URL is not checked when it is set, so the operation used to reach the same dead end
-// without any handler being involved. See #4393.
+// CURLOPT_URL is not checked when it is set, so nothing on this path stops the operation
+// reaching the same dead end with no handler involved at all. See #4393.
 TEST_F(BasicCurlHttpTests, InvalidUrlCompletes)
 {
   auto session_manager = std::make_shared<http_client::curl::HttpCurlClientFactory>()->Create();