Skip to content

Add a way to reset GSDK in cppsdk - #178

Closed
artkuy wants to merge 1 commit into
PlayFab:mainfrom
artkuy:main
Closed

artkuy wants to merge 1 commit into
PlayFab:mainfrom
artkuy:main

Conversation

@artkuy

@artkuy artkuy commented Sep 30, 2024

Copy link
Copy Markdown

This PR modifies the cppsdk to add a way to stop GSDK before calling exit() or exiting main.

Allows user to control lifetime of GSDK, needed to prevent allocations from happening after custom memory allocator has been destroyed. This is done by GSDK::logMessage from the heartbeat thread.

Adds a way to stop gsdk before calling exit() or exiting main
@dgkanatsios

Copy link
Copy Markdown
Contributor

hey artkuy thanks for the contribution. This looks like a change in the public GSDK API, which makes it hard to accept since we'll have to change all other implementations as well.

Can you shed more light on the issue your PR tries to solve? Maybe there's something else we can do on the C++ side to prevent this issue from happening.

@artkuy

artkuy commented Sep 30, 2024

Copy link
Copy Markdown
Author

Hi Dimitris Gkanatsios (@dgkanatsios), in our use case it's a problem that static std::unique_ptr<GSDKInternal> m_instance; is static. Since it won't deconstruct itself until after main is exited or exit() is called. When new and delete are overridden and a custom allocator is used it is possible for an exit crash to occur in std::locale from GSDK::logMessage called from the heartbeat thread. Because GSDK is destroyed after the custom allocator.

We need a way to control the lifetime of GSDK and stop it before we destroy our memory allocator.

@dgkanatsios

Copy link
Copy Markdown
Contributor

thank you for the explanation - I have asked C++ experts in our team to take a look at it.
Quick question: you mention the crash can be caused by the logMessage method - is there any chance we can change that method to prevent the crash?

@artkuy

artkuy commented Oct 1, 2024

Copy link
Copy Markdown
Author

Perhaps, but this seems rather prone to future issues as it's any dynamic allocation that would cause crashing. So any future addition to the project could quickly break it again.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds explicit GSDK lifecycle teardown to stop background work before application shutdown.

Changes:

  • Adds the public GSDK::reset() API.
  • Stops and joins the heartbeat thread before resetting internal state.
  • Guards thread joining with joinable().

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
cpp/cppsdk/gsdk.h Declares and documents reset().
cpp/cppsdk/gsdk.cpp Implements heartbeat shutdown and instance reset.
Suppressed comments (2)

cpp/cppsdk/gsdk.cpp:522

  • reset() can block for the full agent-provided heartbeat interval (which is not capped) because the heartbeat thread may be waiting on m_signalHeartbeatEvent. When that wait eventually expires, the loop also sends one more heartbeat before observing the false flag. Signal the event during reset and re-check m_keepHeartbeatRunning immediately after Wait() so teardown wakes promptly without issuing another request.
                GSDKInternal::get().m_keepHeartbeatRunning = false;

                if (GSDKInternal::get().m_heartbeatThread.joinable())
                {
                    GSDKInternal::get().m_heartbeatThread.join();

cpp/cppsdk/gsdk.cpp:525

  • A shutdown callback is executed by m_shutdownThread; if that callback calls this new teardown API before exiting, resetting the instance destroys the std::future for the currently executing async task. The future destructor waits for that task to finish, so the callback deadlocks waiting on itself. Keep callback-task ownership outside GSDKInternal, or defer instance destruction when reset is invoked from the callback thread.
                GSDKInternal::m_instance.reset();

Comment thread cpp/cppsdk/gsdk.cpp
GSDKInternal::get();
}

void GSDK::reset()
Comment thread cpp/cppsdk/gsdk.cpp
Comment on lines +144 to +147
if (m_heartbeatThread.joinable())
{
m_heartbeatThread.join();
}
@dgkanatsios

Copy link
Copy Markdown
Contributor

Closing in favor of new PR #208.

Dimitris Gkanatsios (dgkanatsios) added a commit that referenced this pull request Oct 3, 2026
* Stop the C++ GSDK's threads cleanly at process exit

The heartbeat thread was only stopped during static destruction, after the
game's own exit-time cleanup. It then waited out the heartbeat interval
(delaying exit), sent one more heartbeat, called the game's health callback
and could log into the already-destroyed log file, and could allocate or
free through a custom allocator that was already gone (#178). Calling
exit() from the shutdown callback, as all C++ test apps do, aborted the
process: the std::async future destroyed itself on its own thread.

- Stop our threads from an atexit handler registered when GSDK starts, and
  wake the heartbeat thread instead of waiting out the interval.
- Run the shutdown callback on a std::thread using `this` rather than get(),
  and never join a thread from itself.
- Never destroy the singleton or the log stream at exit.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

* End the C++ test apps with quick_exit from their shutdown callbacks

With GSDK no longer aborting during exit, calling std::exit() from the
shutdown callback runs the full exit sequence, which deadlocked in these
apps (found by running them under LocalMultiplayerAgent):
- cppLinuxTestApp: main() is blocked in getchar(), and glibc's exit-time
  stdio cleanup waits for it to release stdin.
- cppLinuxRunnerGame: destroying the static processRequestsThread future
  waits for processRequests(), which never returns.
The Windows apps follow the same patterns. Document the pitfall on
registerShutdownCallback.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants