Skip to content

Commit 4178021

Browse files
bmehta001Copilot
andcommitted
Clean up runtime cancellation and logging follow-ups
Address pre-existing follow-ups surfaced during PR review: - TransmissionPolicyManager: fix three LOG_TRACE calls that used %d for uint64_t / chrono::milliseconds::rep values. The negative-delay log was already corrected; this extends the same fix to the WAIT / SCHED / reschedule traces so all delay logging is portable. - WorkerThread: remove the dead 'count' member. It was incremented in Join() and Queue() but never read or decremented anywhere. - HttpClient_Apple::CancelAllRequests: cancel each in-flight request under a single critical section instead of collecting ids, releasing the mutex, and calling CancelRequestAsync per id. The previous pattern opened a small window where the request could complete and be erased between the two locked sections. The spin-wait that follows now also meaningfully blocks until each NSURLSession completion handler has fired and the corresponding HttpRequestApple destructor has removed itself via HttpClient_Apple::Erase, which is the actual signal that cancellation has settled. Validation: - Host UnitTests on macOS arm64: 488/488 pass. - TransmissionPolicyManagerTests + HttpClientManagerTests + HttpResponseDecoderTests --gtest_repeat=10: 46/46 each round. - iphonesimulator and iphoneos -fsyntax-only of HttpClient_Apple.mm both clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 05bd377 commit 4178021

3 files changed

Lines changed: 16 additions & 12 deletions

File tree

lib/http/HttpClient_Apple.mm

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -186,17 +186,19 @@ void Cancel()
186186

187187
void HttpClient_Apple::CancelAllRequests()
188188
{
189-
std::vector<std::string> ids;
190189
{
191190
std::lock_guard<std::mutex> lock(m_requestsMtx);
192-
for (auto const& item : m_requests) {
193-
ids.push_back(item.first);
191+
for (auto const& item : m_requests)
192+
{
193+
auto* request = static_cast<HttpRequestApple*>(item.second);
194+
if (request != nullptr)
195+
{
196+
LOG_TRACE("HTTP request=%p id=%s being aborted...", request, item.first.c_str());
197+
request->Cancel();
198+
}
194199
}
195200
}
196201

197-
for (const auto &id : ids)
198-
CancelRequestAsync(id);
199-
200202
while (true)
201203
{
202204
{

lib/pal/WorkerThread.cpp

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ namespace PAL_NS_BEGIN {
3838
Event m_event;
3939
MAT::Task* m_itemInProgress;
4040
bool m_shuttingDown = false;
41-
int count = 0;
4241

4342
public:
4443

@@ -63,7 +62,6 @@ namespace PAL_NS_BEGIN {
6362
if (!m_shuttingDown) {
6463
m_shuttingDown = true;
6564
m_queue.push_back(new WorkerThreadShutdownItem());
66-
count++;
6765
m_event.post();
6866
}
6967
}
@@ -126,7 +124,6 @@ namespace PAL_NS_BEGIN {
126124
else {
127125
m_queue.push_back(item);
128126
}
129-
count++;
130127
m_event.post();
131128
}
132129

lib/tpm/TransmissionPolicyManager.cpp

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,9 @@ namespace MAT_NS_BEGIN {
164164
// Don't need to cancel and reschedule if it's about to happen now anyways.
165165
// the completion of upload will schedule more uploads as-needed, we only
166166
// want to avoid the unnecessary wasteful rescheduling.
167-
LOG_TRACE("WAIT upload %d ms for lat=%d", delta, m_runningLatency);
167+
LOG_TRACE("WAIT upload %llu ms for lat=%d",
168+
static_cast<unsigned long long>(delta),
169+
static_cast<int>(m_runningLatency));
168170
return;
169171
}
170172
}
@@ -200,7 +202,9 @@ namespace MAT_NS_BEGIN {
200202
m_isUploadScheduled = true;
201203
m_scheduledUploadTime = PAL::getMonotonicTimeMs() + delay.count();
202204
m_runningLatency = latency;
203-
LOG_TRACE("SCHED upload %d ms for lat=%d", delay.count(), m_runningLatency);
205+
LOG_TRACE("SCHED upload %lld ms for lat=%d",
206+
static_cast<long long>(delay.count()),
207+
static_cast<int>(m_runningLatency));
204208
m_scheduledUpload = PAL::scheduleTask(&m_taskDispatcher, static_cast<unsigned>(delay.count()), this, &TransmissionPolicyManager::uploadAsync, latency);
205209
}
206210
}
@@ -264,7 +268,8 @@ namespace MAT_NS_BEGIN {
264268
// Rescheduling upload
265269
if (nextUpload.count() >= 0)
266270
{
267-
LOG_TRACE("Scheduling upload in %d ms", nextUpload.count());
271+
LOG_TRACE("Scheduling upload in %lld ms",
272+
static_cast<long long>(nextUpload.count()));
268273
EventLatency proposed = calculateNewPriority();
269274
scheduleUpload(nextUpload, proposed); // reschedule uploadAsync again
270275
}

0 commit comments

Comments
 (0)