Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[draft] steamcompmgr: attempt to fix the OTHER hang at exit #1652

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/Backends/SDLBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ namespace gamescope
GAMESCOPE_SDL_EVENT_CURSOR,

GAMESCOPE_SDL_EVENT_COUNT,
GAMESCOPE_SDL_EVENT_REQ_EXIT,
};

class CSDLConnector final : public IBackendConnector
Expand Down Expand Up @@ -113,6 +114,7 @@ namespace gamescope
{
public:
CSDLBackend();
~CSDLBackend();

/////////////
// IBackend
Expand Down Expand Up @@ -515,6 +517,11 @@ namespace gamescope
// Do nothing.
}

CSDLBackend::~CSDLBackend() {
PushUserEvent(GAMESCOPE_SDL_EVENT_REQ_EXIT);
m_SDLThread.join();
}

void CSDLBackend::SDLThreadFunc()
{
pthread_setname_np( pthread_self(), "gamescope-sdl" );
Expand Down Expand Up @@ -912,6 +919,9 @@ namespace gamescope

SDL_SetCursor( m_pCursor );
}
else if ( event.type == GetUserEventIndex( GAMESCOPE_SDL_EVENT_REQ_EXIT ) ) {
return;
}
}
break;
}
Expand Down
66 changes: 66 additions & 0 deletions src/Utils/Lock.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
#pragma once
#include "Utils/NonCopyable.h"
#include <memory>
#include <mutex>
#include <optional>
#include <cassert>

namespace gamescope
{
class CGlobalLockReference;
template <CGlobalLockReference const&>
class ReferencedLock;
class CGlobalLockReference : public NonCopyable
{
template <CGlobalLockReference const&>
friend class ReferencedLock;
public:
inline std::optional<std::unique_lock<std::mutex>> popLock() const;
CGlobalLockReference() = default;
CGlobalLockReference(CGlobalLockReference&&) = delete;
CGlobalLockReference(CGlobalLockReference const&&) = delete;
protected:
mutable std::optional<std::unique_lock<std::mutex>>* m_pLock;
mutable std::thread::id m_owningThreadID;
};

template <CGlobalLockReference const& StaticGlobLoc> //a reference to a static object is actually constexpr, which is nice, because putting the reference-to-static in the template param means this class takes up a bit less space
class ReferencedLock : public NonCopyable
{
using T = std::unique_lock<std::mutex>;
friend class CGlobalLockReference;
public:
explicit ReferencedLock(std::mutex& mut) : m_lock{std::in_place_t{}, std::unique_lock{mut}}
{
StaticGlobLoc.m_pLock = &(this->m_lock);
StaticGlobLoc.m_owningThreadID = std::this_thread::get_id();
}

//should still be able to move ReferencedLock when move elision can be done,
//but otherwise don't want to risk messing something up
ReferencedLock(ReferencedLock&&) = delete;
ReferencedLock(ReferencedLock const&&) = delete;

~ReferencedLock() {
if (m_lock.has_value()) {
StaticGlobLoc.m_pLock = nullptr;
}
}
inline std::optional<T> popLock() {
StaticGlobLoc.m_pLock = nullptr;
return std::move(m_lock);
}
protected:
mutable std::optional<T> m_lock;
};

std::optional<std::unique_lock<std::mutex>> CGlobalLockReference::popLock() const {
assert(m_owningThreadID == std::this_thread::get_id());

if (m_pLock) {
return std::move(*std::exchange(m_pLock,nullptr));
} else {
return std::nullopt;
}
}
}
22 changes: 11 additions & 11 deletions src/backend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,34 +16,34 @@ namespace gamescope
// IBackend
/////////////

static IBackend *s_pBackend = nullptr;
static std::atomic<IBackend*> s_pBackend = nullptr;

IBackend *IBackend::Get()
{
return s_pBackend;
}



bool IBackend::Set( IBackend *pBackend )
{
if ( s_pBackend )
{
delete s_pBackend;
s_pBackend = nullptr;
}
//FIXME: calling the backend's destructor sometimes hangs gamescope for wayland backend
//if ( s_pBackend ) {
// std::destroy_at(GetBackend());
//}

if ( pBackend )
{
s_pBackend = pBackend;
if ( !s_pBackend->Init() )
if ( !GetBackend()->Init() )
{
delete s_pBackend;
s_pBackend = nullptr;
s_pBackend = nullptr;
return false;
}
}

return true;
}


/////////////////
// CBaseBackendFb
Expand Down Expand Up @@ -184,4 +184,4 @@ namespace gamescope

GetBackend()->DumpDebugInfo();
});
}
}
9 changes: 9 additions & 0 deletions src/rendervulkan.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,15 @@ struct gamescope_color_mgmt_luts
bHasLut1D = false;
bHasLut3D = false;
}
~gamescope_color_mgmt_luts()
{
//need to set these to nullptr when destructing
//because otherwise, ASAN reports a use-after-free when the exit-time dtor runs
//(first steamcompmgr_exit() destructs this class,
// then the class is destructed again w/ the exit-time dtor for static objects)
vk_lut3d = nullptr;
vk_lut1d = nullptr;
}
};

struct gamescope_color_mgmt_tracker_t
Expand Down
36 changes: 29 additions & 7 deletions src/steamcompmgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@
#include "BufferMemo.h"
#include "Utils/Process.h"
#include "Utils/Algorithm.h"
#include "Utils/Lock.h"

#include "wlr_begin.hpp"
#include "wlr/types/wlr_pointer_constraints_v1.h"
Expand Down Expand Up @@ -5895,9 +5896,30 @@ error(Display *dpy, XErrorEvent *ev)
return 0;
}


static const gamescope::CGlobalLockReference s_xwlLockReference{};
using XwlLock = gamescope::ReferencedLock<s_xwlLockReference>;

[[noreturn]] static void
steamcompmgr_exit(void)
steamcompmgr_exit(std::optional<std::unique_lock<std::mutex>> lock = std::nullopt)
{

// Need to clear all the vk_lutxd references (which can be tied to backend-allocated memory)
// for the colormgmt globals/statics, to avoid coredump at exit from within the colormgmt exit-time destructors:
for (auto& colorMgmtArr :
{
std::ref(g_ColorMgmtLuts),
std::ref(g_ColorMgmtLutsOverride),
std::ref(g_ScreenshotColorMgmtLuts),
std::ref(g_ScreenshotColorMgmtLutsHDR)
})
{
for (auto& colorMgmt : colorMgmtArr.get())
{
colorMgmt.gamescope_color_mgmt_luts::~gamescope_color_mgmt_luts(); //dtor call also calls all the subobjects' dtors
}
}

g_ImageWaiter.Shutdown();

// Clean up any commits.
Expand All @@ -5922,29 +5944,29 @@ steamcompmgr_exit(void)
{
g_ColorMgmt.pending.appHDRMetadata = nullptr;
g_ColorMgmt.current.appHDRMetadata = nullptr;

s_scRGB709To2020Matrix = nullptr;
for (int i = 0; i < gamescope::GAMESCOPE_SCREEN_TYPE_COUNT; i++)
{
s_MuraCorrectionImage[i] = nullptr;
s_MuraCTMBlob[i] = nullptr;
}
}

if (lock)
lock->unlock();
gamescope::IBackend::Set( nullptr );

wlserver_lock();
wlserver_shutdown();
wlserver_unlock(false);

pthread_exit(NULL);
}

static int
handle_io_error(Display *dpy)
{
xwm_log.errorf("X11 I/O error");
steamcompmgr_exit();
steamcompmgr_exit( s_xwlLockReference.popLock() );
}

static bool
Expand Down Expand Up @@ -7494,7 +7516,7 @@ steamcompmgr_main(int argc, char **argv)

init_runtime_info();

std::unique_lock<std::mutex> xwayland_server_guard(g_SteamCompMgrXWaylandServerMutex);
XwlLock xwayland_server_guard(g_SteamCompMgrXWaylandServerMutex);

// Initialize any xwayland ctxs we have
{
Expand Down Expand Up @@ -8042,7 +8064,7 @@ steamcompmgr_main(int argc, char **argv)
vblank = false;
}

steamcompmgr_exit();
steamcompmgr_exit( xwayland_server_guard.popLock() );
}

void steamcompmgr_send_frame_done_to_focus_window()
Expand Down
Loading