Conversation
haroniti
requested review from
MarcoWilliamsPF,
andhesky,
andrewhau-pf,
pgilmorepf,
ravarnamsft and
tophpalmer
andrewhau-pf
approved these changes
Nov 8, 2018
dgkanatsios
pushed a commit
that referenced
this pull request
Apr 30, 2026
Fixes the four findings from the GitHub Copilot reviewer on PR #97: 1. PlayFabTelemetryClient.cpp - the HTTP completion lambda now captures a TSharedRef (not TWeakPtr) of the client. If the owning subsystem releases its TSharedPtr while a request is in flight (e.g. during Deinitialize), the dequeued batch was previously lost. The strong ref keeps the client alive until the request completes. 2. MpsTelemetrySubsystem.cpp - Deinitialize now calls BeginShutdown when the host did not, so server_metrics_final is always enqueued. Combined with completion chaining (see #1) and the strong-ref capture, in-flight and queued events keep flushing even after the subsystem releases its pointer. 3. PlayFabTelemetryClient.cpp - bFlushInProgress is now read and written under SyncRoot, including a single critical section that performs the in-progress check, dequeue, and flag flip together. Two concurrent FlushAsync calls can no longer both pull a batch and start parallel HTTP requests. The dedicated DequeueBatch helper is removed; ClearFlushInProgress is added so failure paths reliably reset the flag under the lock. 4. PlayFabTelemetryClient.cpp - the failure-path log truncates the response body to 512 chars to keep server logs bounded under sustained 5xx storms. Also: the HTTP completion lambda now performs completion chaining. After clearing bFlushInProgress, if pending events remain (because new ones were enqueued, requeued after a failure, or queued by BeginShutdown after the periodic ticker was removed) it kicks off another FlushAsync. This is what drains the queue during shutdown without relying on a sleep-based poll loop, which would block the same game thread that delivers HTTP completions. Not verified end-to-end on this commit (Unreal Engine has been uninstalled locally). Previous commits in this PR were verified against UE 5.7.4 + VS 2026; these changes are surgical and use only APIs already exercised in this plugin (FCriticalSection / FScopeLock, TSharedRef + AsShared, FString::Left). A teammate with Unreal installed should re-run the smoke test before merge. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adding a README to help users get started with the WinRunner