Fix cancel-race in HttpClient_Android

Resolves #425
2020-06-08 15:36:51 -07:00 · 2020-06-08 15:36:51 -07:00 · dbe86c8609
--- a/lib/android_build/app/src/main/cpp/native-lib.cpp
+++ b/lib/android_build/app/src/main/cpp/native-lib.cpp
@ -64,10 +64,12 @@ public:
    }

    void OnTestStart(const ::testing::TestInfo& test_info) override {
+        auto param = test_info.value_param();
        __android_log_print(
                ANDROID_LOG_INFO,
                "MAE",
-                "Start %s\n",
+                "Start %s.%s\n",
+                test_info.test_case_name(),
                test_info.name()
                );
    }
@ -76,17 +78,21 @@ public:
        __android_log_print(
                ANDROID_LOG_INFO,
                "MAE",
-                "End %s: %s\n",
+                "End %s.%s: %s\n",
+                test_info.test_case_name(),
                test_info.name(),
                test_info.result()->Failed() ? "FAIL" : "OK"
                );
    }

-    void OnTestProgramEnd(const ::testing::UnitTest& /* unit test */) override {
+    void OnTestProgramEnd(const ::testing::UnitTest& unitTest) override {
        __android_log_print(
                ANDROID_LOG_INFO,
                "MAE",
-                "End tests\n"
+                "End tests: %d succeeded, %d failed, %d total\n",
+                unitTest.successful_test_count(),
+                unitTest.failed_test_count(),
+                unitTest.total_test_count()
                );
    }
 };
@ -95,7 +101,8 @@ int RunTests::run_all_tests(JNIEnv * env, jobject java_logger)
 {
    int argc = 1;
    char command_name[] = "maesdk-test";
-    char *argv[] = { command_name };
+    char filter[] = "--gtest_filter=Storage/*";
+    char *argv[] = { command_name, filter };
    ::testing::InitGoogleTest(&argc, argv);
    ::testing::TestEventListeners& listeners =
            ::testing::UnitTest::GetInstance()->listeners();
--- a/lib/http/HttpClient_Android.cpp
+++ b/lib/http/HttpClient_Android.cpp
@ -103,6 +103,13 @@ void HttpClient_Android::SendRequestAsync(IHttpRequest* request,
 				{
 					r = u;
 					r->m_callback = callback;
+					bool valid = r->m_state == RequestState::early || r->m_state == RequestState::cancel;
+					if (!valid) {
+						throw std::logic_error("neither early nor cancel");
+					}
+					if (r->m_state == RequestState::early) {
+						r->m_state = RequestState::preparing;
+					}
 				}
 				break;
 			}
@ -113,6 +120,7 @@ void HttpClient_Android::SendRequestAsync(IHttpRequest* request,
 	{
 		return;
 	}
+
 	HttpHeaders const& headers = r->GetHeaders();

 	size_t total_size = 0;
@ -153,13 +161,23 @@ void HttpClient_Android::SendRequestAsync(IHttpRequest* request,
 	auto java_id = env->NewStringUTF(request->GetId().c_str());
 	auto url_id = env->NewStringUTF(r->m_url.c_str());
 	auto method_id = env->NewStringUTF(r->m_method.c_str());
-	auto result = env->CallObjectMethod(m_client, m_create_id,
-		url_id,
-		method_id,
-		body,
-		java_id,
-		header_lengths,
-		header_buffer);
+	bool proceed = true;
+	{
+		std::lock_guard<std::mutex> lock(m_requestsMutex);
+		proceed = r->m_state == RequestState::preparing;
+	}
+	jobject result = nullptr;
+	if (proceed) {
+		result = env->CallObjectMethod(
+			m_client,
+			m_create_id,
+			url_id,
+			method_id,
+			body,
+			java_id,
+			header_lengths,
+			header_buffer);
+	}
 	if (pushed == JNI_OK)
 	{
        result = env->PopLocalFrame(result);
@ -172,14 +190,19 @@ void HttpClient_Android::SendRequestAsync(IHttpRequest* request,
 		{
 			if (u->m_id == target_id)
 			{
-				u->m_callback = callback;
-				u->m_java_request = env->NewGlobalRef(result);
-				if (!result || u->m_cancel_request)
+				if (u->m_callback != callback) {
+					throw std::logic_error("callback");
+				}
+				if (!result || u->m_state != RequestState::preparing)
 				{
 					cancel_me = u;
 					u = m_requests.back();
 					m_requests.pop_back();
 				}
+				else {
+					u->m_java_request = env->NewGlobalRef(result);
+					u->m_state = RequestState::running;
+				}
 				break;
 			}
 		}
@ -223,26 +246,31 @@ void HttpClient_Android::CancelRequestAsync(std::string const& id)
 		{
 			if (u->m_id == id)
 			{
-				if (u->m_cancel_request)
-				{
-					return; // someone already called cancel for this request
+				switch (u->m_state) {
+					case RequestState::cancel:
+						return; // SendRequestAsync will cancel this request
+					case RequestState::preparing:
+					case RequestState::early:
+						// tell SendRequestAsync to cancel this request
+						u->m_state = RequestState::cancel;
+						return;
+					case RequestState::running:
+						// SendRequestAsync will not cancel this request
+						// We won the race with DispatchCallback
+						cancel_me = u;
+						u = m_requests.back();
+						m_requests.pop_back();
+						break;
+					default:
+						throw std::logic_error("request state");
 				}
-				if (u->m_callback)
-				{
-					cancel_me = u;
-				}
-				else
-				{
-					u->m_cancel_request = true;
-				}
-				break;
 			}
 		}
 	}

 	if (cancel_me)
 	{
-		CallbackForCancel(env, std::move(cancel_me));
+		CallbackForCancel(env, cancel_me);
 	}
 }

@ -253,37 +281,33 @@ void HttpClient_Android::CancelAllRequests()
 	{
 		return;
 	}
-
-	// We will cancel, notify, and destroy all requests, so swap
-	// them into a local vector inside the mutex.
-
-	std::vector<HttpRequest *> local_requests;
+	std::vector<HttpRequest *> toCancel;
 	{
 		std::lock_guard<std::mutex> lock(m_requestsMutex);
-		local_requests.swap(m_requests);
+		std::vector<HttpRequest *>::iterator first_cancel = std::partition(m_requests.begin(), m_requests.end(), [](HttpRequest * request)-> bool
+		{
+			switch(request->m_state) {
+				// return false for running requests to move them to the end of the vector
+				case RequestState::cancel:
+					return true;
+				case RequestState::early:
+				case RequestState::preparing:
+					request->m_state = RequestState::cancel;
+					return true;
+				case RequestState::running:
+					return false;
+				default:
+					return true;
+			}
+		});
+		if (first_cancel != m_requests.end()) {
+			// move running requests to the toCancel vector, remove from m_requests
+			toCancel.assign(first_cancel, m_requests.end());
+			m_requests.erase(first_cancel, m_requests.end());
+		}
 	}
-	jclass request_class = nullptr;
-	jmethodID cancel_method = nullptr;
-	for (const auto& u : local_requests)
-	{
-		if (u->m_java_request)
-		{
-			if (!request_class)
-			{
-				request_class = env->GetObjectClass(u->m_java_request);
-			}
-			if (!cancel_method)
-			{
-				cancel_method = env->GetMethodID(request_class, "cancel", "(Z)Z");
-			}
-			jboolean interrupt_yes = JNI_TRUE;
-			env->CallBooleanMethod(u->m_java_request, cancel_method, interrupt_yes);
-		}
-		if (u->m_callback)
-		{
-			auto failure = new HttpResponse(u->m_id);
-			u->m_callback->OnHttpResponse(failure);
-		}
+	for (auto const & request : toCancel) {
+		CallbackForCancel(env, request);
 	}
 }

@ -318,14 +342,17 @@ void HttpClient_Android::EraseRequest(HttpRequest* request)
 	}
 }

-HttpClient_Android::HttpRequest* HttpClient_Android::GetRequest(std::string id)
+HttpClient_Android::HttpRequest* HttpClient_Android::GetAndRemoveRequest(std::string id)
 {
 	std::lock_guard<std::mutex> lock(m_requestsMutex);
 	for (auto&& u : m_requests)
 	{
 		if (u->m_id == id)
 		{
-			return u;
+			auto r = u;
+			u = m_requests.back();
+			m_requests.pop_back();
+			return r;
 		}
 	}
 	return nullptr;
@ -445,7 +472,7 @@ Java_com_microsoft_applications_events_HttpClient_dispatchCallback(
 	env->ReleaseStringUTFChars(id, id_utf);
 	using HttpClient_Android = Microsoft::Applications::Events::HttpClient_Android;
 	auto client = HttpClient_Android::GetClientInstance();
-	auto request = client->GetRequest(id_string);
+	auto request = client->GetAndRemoveRequest(id_string);
 	if (!request)
 	{
 		return;
@ -473,4 +500,4 @@ Java_com_microsoft_applications_events_HttpClient_dispatchCallback(
 	env->ReleaseByteArrayElements(body, body_pointer, JNI_ABORT);
 	// callback will own response
 	callback->OnHttpResponse(response);
-}
+}
--- a/lib/http/HttpClient_Android.hpp
+++ b/lib/http/HttpClient_Android.hpp
@ -12,6 +12,13 @@ namespace ARIASDK_NS_BEGIN {
 class HttpClient_Android : public IHttpClient
 {

+    enum class RequestState : int8_t {
+        early = 0,
+        preparing,
+        running,
+        cancel
+    };
+
 public:
 	class HttpResponse : public IHttpResponse {

@ -142,7 +149,7 @@ public:
        }

 	protected:
-      HttpClient_Android &    m_parent;
+        HttpClient_Android &    m_parent;
 		HttpHeaders             m_headers;
 		IHttpResponseCallback*  m_callback = nullptr;
 		std::string             m_id;
@ -150,7 +157,7 @@ public:
 		std::string             m_url;
 		std::vector<uint8_t>    m_body;
 		jobject 				m_java_request = nullptr;
-		bool					m_cancel_request = false;
+		RequestState			m_state = RequestState::early;

 		friend HttpClient_Android;
 	};
@ -164,7 +171,7 @@ public:
 	void CancelAllRequests() override;
 	void SetClient(JNIEnv* env, jobject c);
 	void EraseRequest(HttpRequest*);
-	HttpRequest* GetRequest(std::string id);
+	HttpRequest* GetAndRemoveRequest(std::string id);
 	std::string NextIdString();

 	static void CreateClientInstance(JNIEnv* env,