diff --git a/lib/http/HttpClient_Apple.mm b/lib/http/HttpClient_Apple.mm index b13d92157..85a4888f8 100644 --- a/lib/http/HttpClient_Apple.mm +++ b/lib/http/HttpClient_Apple.mm @@ -378,7 +378,11 @@ void HandleResponse(NSData* data, NSURLResponse* response, NSError* error) } } - if (cancelRequested) + // A late cancellation must not discard an acknowledgement from a + // completed transfer. Status 200 alone is insufficient if it failed. + const bool completedSuccess = error == nil && httpResp != nil && + simpleResponse->m_statusCode == 200 && !headersTooLarge; + if (cancelRequested && !completedSuccess) { simpleResponse->m_result = HttpResult_Aborted; } @@ -463,7 +467,11 @@ bool Cancel() return m_callback == nullptr && m_dataTask == nil; } -private: + private: +#if defined(MATSDK_APPLE_HTTP_TESTS) + friend MATSDK_LIBABI void CompleteAppleResponseForTest(IHttpResponseCallback*, bool, unsigned, int, size_t); +#endif + void Complete(HttpResult result) { IHttpResponseCallback* callback = nullptr; @@ -517,6 +525,35 @@ void Complete(HttpResult result) bool m_terminal = false; }; +#if defined(MATSDK_APPLE_HTTP_TESTS) +MATSDK_LIBABI void CompleteAppleResponseForTest(IHttpResponseCallback* callback, + bool cancelRequested, + unsigned statusCode, + int errorCode, + size_t headerSize) +{ + @autoreleasepool + { + HttpClient_Apple parent; + HttpRequestApple request(&parent); + request.m_callback = callback; + if (cancelRequested) + { + request.Cancel(); + } + NSString* value = [@"" stringByPaddingToLength:headerSize withString:@"x" startingAtIndex:0]; + NSURL* url = [NSURL URLWithString:@"https://localhost/"]; + NSHTTPURLResponse* response = statusCode == 0 ? nil : [[NSHTTPURLResponse alloc] initWithURL:url + statusCode:statusCode + HTTPVersion:@"HTTP/1.1" + headerFields:@{@"X-Test" : value}]; + NSError* error = errorCode == 0 ? nil : [NSError errorWithDomain:NSURLErrorDomain code:errorCode userInfo:nil]; + NSData* body = [@"{\"acc\":1}" dataUsingEncoding:NSUTF8StringEncoding]; + request.HandleResponse(body, response, error); + } +} +#endif + HttpClient_Apple::HttpClient_Apple() { LOG_TRACE("Initializing HttpClient_Apple..."); diff --git a/tests/unittests/CMakeLists.txt b/tests/unittests/CMakeLists.txt index 84a73b4ce..ff682427b 100644 --- a/tests/unittests/CMakeLists.txt +++ b/tests/unittests/CMakeLists.txt @@ -1,5 +1,10 @@ message(STATUS "Building unittests") +if(APPLE AND PAL_IMPLEMENTATION STREQUAL "CPP11" + AND (MATSDK_BUILD_APPLE_HTTP OR MATSDK_PLATFORM_IOS)) + target_compile_definitions(mat PRIVATE MATSDK_APPLE_HTTP_TESTS) +endif() + set(SRCS AIJsonSerializerTests.cpp AITelemetrySystemTests.cpp diff --git a/tests/unittests/HttpClientTests.cpp b/tests/unittests/HttpClientTests.cpp index 06cb861b3..808cf8917 100644 --- a/tests/unittests/HttpClientTests.cpp +++ b/tests/unittests/HttpClientTests.cpp @@ -581,6 +581,67 @@ TEST_F(HttpClientTests, HandlesLocalErrors) } #if defined(MAT_TEST_APPLE_TRANSPORT) +namespace MAT_NS_BEGIN +{ + void CompleteAppleResponseForTest(IHttpResponseCallback* callback, + bool cancelRequested, + unsigned statusCode, + int errorCode, + size_t headerSize); +} +MAT_NS_END + +TEST_F(HttpClientTests, CompletedAppleResponseSurvivesPendingCancellation) +{ + CompleteAppleResponseForTest(this, true, 200, 0, 4); + + ASSERT_EQ(_responses.size(), 1u); + EXPECT_EQ(_responses[0]->GetResult(), HttpResult_OK); + EXPECT_EQ(_responses[0]->GetStatusCode(), 200u); + EXPECT_EQ(_responses[0]->GetHeaders().get("X-Test"), "xxxx"); + const auto& body = _responses[0]->GetBody(); + EXPECT_EQ(std::string(body.begin(), body.end()), "{\"acc\":1}"); +} + +TEST_F(HttpClientTests, PendingAppleCancellationDoesNotAcceptUnconfirmedResponses) +{ + struct ResponseCase + { + unsigned status; + int error; + size_t headerSize; + }; + const ResponseCase cases[] = { + {200, -999, 4}, // NSURLErrorCancelled + {200, -1005, 4}, // NSURLErrorNetworkConnectionLost + {0, 0, 4}, + {200, 0, MAX_HTTP_RESPONSE_HEADERS_SIZE}, + {201, 0, 4}, + {500, 0, 4}, + }; + for (const auto& test : cases) + { + SCOPED_TRACE(::testing::Message() << test.status << ", " << test.error << ", " << test.headerSize); + Clear(); + CompleteAppleResponseForTest(this, true, test.status, test.error, test.headerSize); + ASSERT_EQ(_responses.size(), 1u); + EXPECT_EQ(_responses[0]->GetResult(), HttpResult_Aborted); + EXPECT_TRUE(_responses[0]->GetBody().empty()); + } +} + +TEST_F(HttpClientTests, AppleResponseValidationWithoutCancellationIsUnchanged) +{ + CompleteAppleResponseForTest(this, false, 200, 0, 4); + ASSERT_EQ(_responses.size(), 1u); + EXPECT_EQ(_responses[0]->GetResult(), HttpResult_OK); + + Clear(); + CompleteAppleResponseForTest(this, false, 200, 0, MAX_HTTP_RESPONSE_HEADERS_SIZE); + ASSERT_EQ(_responses.size(), 1u); + EXPECT_EQ(_responses[0]->GetResult(), HttpResult_NetworkFailure); +} + TEST_F(HttpClientTests, InvalidUtf8UrlCompletesExactlyOnce) { // The request must outlive the whole exchange: keep ownership here (the Apple