diff --git a/lib/tdeck_ui/Hardware/TDeck/MapTileDownloader.cpp b/lib/tdeck_ui/Hardware/TDeck/MapTileDownloader.cpp index 90ae87ea..cb25df18 100644 --- a/lib/tdeck_ui/Hardware/TDeck/MapTileDownloader.cpp +++ b/lib/tdeck_ui/Hardware/TDeck/MapTileDownloader.cpp @@ -35,7 +35,8 @@ MapTileDownloader::MapTileDownloader(MapTileDownloadStore& store, MapTileTranspo : store_(store), transport_(transport), clock_(clock), policy_(policy), config_(config), queue_{}, queue_count_(0U), results_{}, result_head_(0U), result_count_(0U), dropped_results_(0U), current_{}, active_(false), transport_open_(false), - store_open_(false), stage_(Stage::SELECTED), last_now_(0U), deadline_(0U), + transport_retry_used_(false), store_open_(false), stage_(Stage::SELECTED), + last_now_(0U), deadline_(0U), received_(0U), expected_length_(-1), url_{0}, user_agent_{0}, chunk_{0} {} MapTileDownloader::~MapTileDownloader() { @@ -189,6 +190,7 @@ MapTilePumpResult MapTileDownloader::pump() { received_ = 0U; expected_length_ = -1; transport_open_ = false; + transport_retry_used_ = false; store_open_ = false; stage_ = Stage::SELECTED; last_now_ = clock_.nowMs(); @@ -220,6 +222,17 @@ MapTilePumpResult MapTileDownloader::pump() { const TileTransportResult started = transport_.start(url_, user_agent_, config_.ca_certificate, config_.connect_timeout_ms, config_.read_timeout_ms, response); if (started != TileTransportResult::OK) { + // HTTPClient may retain a server-closed keep-alive socket when reuse + // is enabled. Force the concrete transport to release its socket and + // TLS context before one bounded reconnect attempt. + transport_.reset(); + // A blocking reconnect attempt can consume the remaining overall + // budget even when it returns a generic transport error. + if (!checkClock()) return MapTilePumpResult::PROGRESSED; + if (started == TileTransportResult::ERROR && !transport_retry_used_) { + transport_retry_used_ = true; + return MapTilePumpResult::PROGRESSED; + } finish(started == TileTransportResult::TIMEOUT ? MapTileResultCode::TIMEOUT : MapTileResultCode::TRANSPORT_ERROR, false); return MapTilePumpResult::PROGRESSED; diff --git a/lib/tdeck_ui/Hardware/TDeck/MapTileDownloader.h b/lib/tdeck_ui/Hardware/TDeck/MapTileDownloader.h index 0e64aa9b..0ac7815a 100644 --- a/lib/tdeck_ui/Hardware/TDeck/MapTileDownloader.h +++ b/lib/tdeck_ui/Hardware/TDeck/MapTileDownloader.h @@ -45,6 +45,8 @@ public: virtual TileTransportResult read(std::uint8_t* output, std::size_t capacity, std::size_t& count, bool& eof) = 0; virtual void close() = 0; + /** Hard-reset transport state before a bounded reconnect attempt. */ + virtual void reset() { close(); } }; class MapTileDownloadClock { @@ -97,8 +99,9 @@ struct MapTileDownloadResult { /** * Fixed-capacity, caller-pumped downloader for visible slippy-map tiles. * - * Requests contain only TileKey + generation. There is no retry, prefetch, - * background bulk mode, credential support, or hidden URL input. Keep one + * Requests contain only TileKey + generation. A failed transport start gets one + * hard-reset reconnect attempt; there is no content retry, prefetch, background + * bulk mode, credential support, or hidden URL input. Keep one * visible tile request active at a time. Users of the default public endpoint * must preserve visible OpenStreetMap attribution in the eventual map UI and * comply with https://operations.osmfoundation.org/policies/tiles/ . @@ -149,6 +152,7 @@ private: Request current_; bool active_; bool transport_open_; + bool transport_retry_used_; bool store_open_; Stage stage_; std::uint64_t last_now_; diff --git a/lib/tdeck_ui/Hardware/TDeck/MapTileHttpArduino.cpp b/lib/tdeck_ui/Hardware/TDeck/MapTileHttpArduino.cpp index 9cfa325d..c7ef4aa4 100644 --- a/lib/tdeck_ui/Hardware/TDeck/MapTileHttpArduino.cpp +++ b/lib/tdeck_ui/Hardware/TDeck/MapTileHttpArduino.cpp @@ -21,7 +21,7 @@ std::int32_t boundedConnectTimeout(std::uint32_t value) { MapTileHttpArduino::MapTileHttpArduino() : stream_(NULL), remaining_(-1), open_(false), content_type_{0} {} -MapTileHttpArduino::~MapTileHttpArduino() { close(); } +MapTileHttpArduino::~MapTileHttpArduino() { reset(); } TileTransportResult MapTileHttpArduino::start(const char* url, const char* user_agent, const char* ca_certificate, std::uint32_t connect_timeout_ms, @@ -103,13 +103,24 @@ void MapTileHttpArduino::close() { content_type_[0] = '\0'; } -void MapTileHttpArduino::disconnectIdle() { - client_.stop(); +void MapTileHttpArduino::reset() { + // Disable reuse so HTTPClient::end() stops an active secure connection. + // Do not stop the secure client directly: it may already have done so after + // a connect/write failure, and this framework zeroes the context afterward, + // making a second stop interpret socket 0 as live. + http_.setReuse(false); http_.end(); + http_.detachClient(); + client_.markStopped(); stream_ = NULL; remaining_ = -1; open_ = false; content_type_[0] = '\0'; + http_.setReuse(true); +} + +void MapTileHttpArduino::disconnectIdle() { + reset(); } MapTileMillisClock::MapTileMillisClock() : previous_(millis()), high_(0U) {} diff --git a/lib/tdeck_ui/Hardware/TDeck/MapTileHttpArduino.h b/lib/tdeck_ui/Hardware/TDeck/MapTileHttpArduino.h index 466e1ba6..07e8dfe1 100644 --- a/lib/tdeck_ui/Hardware/TDeck/MapTileHttpArduino.h +++ b/lib/tdeck_ui/Hardware/TDeck/MapTileHttpArduino.h @@ -14,6 +14,18 @@ namespace Hardware { namespace TDeck { +class MapTileSecureClient : public WiFiClientSecure { +public: + /** Restore the framework's stopped-socket sentinel after TLS cleanup. */ + void markStopped() { if (sslclient != NULL) sslclient->socket = -1; } +}; + +class MapTileHttpClient : public HTTPClient { +public: + /** Prevent HTTPClient destruction from stopping an already-stopped client. */ + void detachClient() { _client = NULL; } +}; + /** HTTPS-only transport. A non-empty explicit CA is mandatory for peer verification. */ class MapTileHttpArduino : public MapTileTransport { public: @@ -25,10 +37,11 @@ public: virtual TileTransportResult read(std::uint8_t* output, std::size_t capacity, std::size_t& count, bool& eof); virtual void close(); + virtual void reset(); void disconnectIdle(); private: - WiFiClientSecure client_; - HTTPClient http_; + MapTileSecureClient client_; + MapTileHttpClient http_; WiFiClient* stream_; std::int64_t remaining_; bool open_; diff --git a/tests/build_scripts/test_map_tile_downloader_contract.py b/tests/build_scripts/test_map_tile_downloader_contract.py index a6d0bf1b..7f6a70bc 100644 --- a/tests/build_scripts/test_map_tile_downloader_contract.py +++ b/tests/build_scripts/test_map_tile_downloader_contract.py @@ -39,7 +39,15 @@ def test_https_adapter_verifies_peer_with_explicit_ca_and_has_no_credentials(): assert "setReuse(true)" in source assert "useHTTP10(true)" not in source assert "disconnectIdle" in source - assert "client_.stop()" in source + reset = ADAPTER_CPP.read_text().split("void MapTileHttpArduino::reset()", 1)[1].split("}\n", 1)[0] + assert "http_.setReuse(false)" in reset + assert "http_.end()" in reset + assert "http_.detachClient()" in reset + assert "client_.markStopped()" in reset + assert "MapTileHttpArduino::~MapTileHttpArduino() { reset(); }" in source + # The pinned WiFiClientSecure zeroes its socket context after an internal + # failure stop; an explicit second stop can therefore close descriptor 0. + assert "client_.stop()" not in reset for forbidden in ("Authorization", "Cookie", "username", "password", "SD.begin", "format(", "LittleFS"): assert forbidden not in source diff --git a/tests/native/test_map_tile_downloader.cpp b/tests/native/test_map_tile_downloader.cpp index c938095b..0fb12a04 100644 --- a/tests/native/test_map_tile_downloader.cpp +++ b/tests/native/test_map_tile_downloader.cpp @@ -59,6 +59,7 @@ public: class FakeTransport : public MapTileTransport { public: TileTransportResult start_result; + std::vector start_results; TileTransportResult read_result; int status; std::int64_t length; @@ -75,16 +76,19 @@ public: std::uint32_t connect_timeout; std::uint32_t read_timeout; FakeClock* clock; + std::uint64_t advance_on_start; std::uint64_t advance_on_read; FakeTransport() : start_result(TileTransportResult::OK), read_result(TileTransportResult::OK), status(200), length(-1), type("image/png"), position(0U), forced_chunk(0U), starts(0), reads(0), closes(0), - connect_timeout(0U), read_timeout(0U), clock(NULL), advance_on_read(0U) {} + connect_timeout(0U), read_timeout(0U), clock(NULL), advance_on_start(0U), advance_on_read(0U) {} virtual TileTransportResult start(const char* u, const char* a, const char* c, std::uint32_t ct, std::uint32_t rt, TileHttpResponse& response) { ++starts; url = u == NULL ? "" : u; agent = a == NULL ? "" : a; ca = c == NULL ? "" : c; + if (clock != NULL) clock->value += advance_on_start; connect_timeout = ct; read_timeout = rt; position = 0U; response.status_code = status; response.content_length = length; response.content_type = type; - return start_result; + const std::size_t attempt = static_cast(starts - 1); + return attempt < start_results.size() ? start_results[attempt] : start_result; } virtual TileTransportResult read(std::uint8_t* output, std::size_t capacity, std::size_t& count, bool& eof) { ++reads; @@ -147,6 +151,58 @@ void testTransportAndStoreFailuresAbort() { beginTest(); { FakeStore s; s.short_write=true; FakeTransport t; t.body=bytes(30U); FakeClock c; MapTileDownloader d(s,t,c,enabled(),config()); CHECK(d.enqueue(key(),1U)==MapTileEnqueueResult::ACCEPTED); runUntilIdle(d,c); CHECK(take(d).code==MapTileResultCode::STORE_ERROR); CHECK(s.aborts==1); } { FakeStore s; s.finish_result=TileStoreResult::IO_ERROR; FakeTransport t; t.body=bytes(30U); FakeClock c; MapTileDownloader d(s,t,c,enabled(),config()); CHECK(d.enqueue(key(),1U)==MapTileEnqueueResult::ACCEPTED); runUntilIdle(d,c); CHECK(take(d).code==MapTileResultCode::STORE_ERROR); CHECK(s.aborts==1); } } +void testTransportStartFailureHardClosesAndRetriesOnce() { beginTest(); + FakeStore s; FakeTransport t; t.start_results.push_back(TileTransportResult::ERROR); + t.start_results.push_back(TileTransportResult::OK); t.body=bytes(30U); + FakeClock c; MapTileDownloader d(s,t,c,enabled(),config()); + CHECK(d.enqueue(key(),1U)==MapTileEnqueueResult::ACCEPTED); runUntilIdle(d,c); + CHECK(take(d).code==MapTileResultCode::SUCCESS); CHECK(t.starts==2); CHECK(t.closes==2); +} +void testTransportStartRetryIsBounded() { beginTest(); + FakeStore s; FakeTransport t; t.start_result=TileTransportResult::ERROR; + FakeClock c; MapTileDownloader d(s,t,c,enabled(),config()); + CHECK(d.enqueue(key(),1U)==MapTileEnqueueResult::ACCEPTED); runUntilIdle(d,c); + CHECK(take(d).code==MapTileResultCode::TRANSPORT_ERROR); CHECK(t.starts==2); CHECK(t.closes==2); +} +void testTransportStartTimeoutIsNotRetried() { beginTest(); + FakeStore s; FakeTransport t; t.start_result=TileTransportResult::TIMEOUT; + FakeClock c; MapTileDownloader d(s,t,c,enabled(),config()); + CHECK(d.enqueue(key(),1U)==MapTileEnqueueResult::ACCEPTED); runUntilIdle(d,c); + CHECK(take(d).code==MapTileResultCode::TIMEOUT); CHECK(t.starts==1); CHECK(t.closes==1); +} +void testTransportRetryHonorsOverallDeadline() { beginTest(); + FakeStore s; FakeTransport t; t.start_result=TileTransportResult::ERROR; + FakeClock c; t.clock=&c; t.advance_on_start=20U; MapTileDownloadConfig cfg=config(); + cfg.overall_timeout_ms=10U; MapTileDownloader d(s,t,c,enabled(),cfg); + CHECK(d.enqueue(key(),1U)==MapTileEnqueueResult::ACCEPTED); runUntilIdle(d,c); + CHECK(take(d).code==MapTileResultCode::TIMEOUT); CHECK(t.starts==1); CHECK(t.closes==1); +} +void testSecondTransportFailureHonorsOverallDeadline() { beginTest(); + FakeStore s; FakeTransport t; t.start_result=TileTransportResult::ERROR; + FakeClock c; t.clock=&c; t.advance_on_start=6U; MapTileDownloadConfig cfg=config(); + cfg.overall_timeout_ms=10U; MapTileDownloader d(s,t,c,enabled(),cfg); + CHECK(d.enqueue(key(),1U)==MapTileEnqueueResult::ACCEPTED); runUntilIdle(d,c); + CHECK(take(d).code==MapTileResultCode::TIMEOUT); CHECK(t.starts==2); CHECK(t.closes==2); +} +void testTransportRetryCanBeCanceledBetweenAttempts() { beginTest(); + FakeStore s; FakeTransport t; t.start_result=TileTransportResult::ERROR; + FakeClock c; MapTileDownloader d(s,t,c,enabled(),config()); + CHECK(d.enqueue(key(),5U)==MapTileEnqueueResult::ACCEPTED); + CHECK(d.pump()==MapTilePumpResult::PROGRESSED); + CHECK(d.pump()==MapTilePumpResult::PROGRESSED); CHECK(t.starts==1); CHECK(t.closes==1); + CHECK(d.cancelGeneration(5U)==1U); runUntilIdle(d,c); + CHECK(take(d).code==MapTileResultCode::CANCELED); CHECK(t.starts==1); +} +void testTransportRetryBudgetResetsForNextRequest() { beginTest(); + FakeStore s; FakeTransport t; t.start_results.push_back(TileTransportResult::ERROR); + t.start_results.push_back(TileTransportResult::OK); t.start_results.push_back(TileTransportResult::ERROR); + t.start_results.push_back(TileTransportResult::OK); t.body=bytes(30U); + FakeClock c; MapTileDownloader d(s,t,c,enabled(),config()); + CHECK(d.enqueue(key(1U),1U)==MapTileEnqueueResult::ACCEPTED); + CHECK(d.enqueue(key(2U),2U)==MapTileEnqueueResult::ACCEPTED); runUntilIdle(d,c); + CHECK(take(d).code==MapTileResultCode::SUCCESS); CHECK(take(d).code==MapTileResultCode::SUCCESS); + CHECK(t.starts==4); CHECK(t.closes==4); +} void testCancellationAtStagesAndGenerationIsolation() { beginTest(); for(int stage=0;stage<3;++stage){ FakeStore s; FakeTransport t; t.body=bytes(5000U); FakeClock c; MapTileDownloader d(s,t,c,enabled(),config()); CHECK(d.enqueue(key(),5U)==MapTileEnqueueResult::ACCEPTED); for(int i=0;i None: env["UBSAN_OPTIONS"] = "halt_on_error=1:print_stacktrace=1" ran = subprocess.run([str(binary)], capture_output=True, text=True, timeout=60, env=env) assert ran.returncode == 0, ran.stdout + ran.stderr - assert ran.stdout == "map tile downloader: 13 tests passed\n" + assert ran.stdout == "map tile downloader: 20 tests passed\n"