Repository navigation
Fix segfault on window close: let GLFW outlive the Vulkan objects - #443
ozanyasindogan wants to merge 5 commits into
Conversation
`cleanup()` called `glfwDestroyWindow()` and `glfwTerminate()` while every
`vk::raii` member was still alive. Those members belong to the application
class, so they are destroyed only after `run()` returns - that is, after GLFW
has already torn down the window system connection they still reference.
`vkDestroySwapchainKHR` then marshals a request on freed Wayland proxies and
the process dies during exit:
KhronosGroup#12 HelloTriangleApplication::~HelloTriangleApplication()
KhronosGroup#11 vk::raii::SwapchainKHR::~SwapchainKHR()
KhronosGroup#10 vk::raii::SwapchainKHR::clear()
KhronosGroup#4 libnvidia-glcore.so
KhronosGroup#3 wl_proxy_marshal_flags
#0 libwayland-client.so.0 <- SEGV_MAPERR
The pre-RAII tutorial destroyed every Vulkan object by hand at the end of
`cleanup()`, so `glfwTerminate()` genuinely ran last. Converting to `vk::raii`
removed those explicit calls but left the two GLFW calls behind, silently
inverting the order.
Ownership of the GLFW lifetime now belongs to a `GlfwGuard` member declared
first in the class. Members are destroyed in reverse declaration order, so
being first means it is destroyed last - after every Vulkan object, however
many later chapters add. `glfwTerminate()` also destroys any windows still
open, so the separate `glfwDestroyWindow()` call is no longer needed.
Applied to all 34 affected chapters, plus 00_base_code so the pattern is
correct from the very first chapter and never has to change again - which is
what the Base Code chapter already promises the reader.
Verified on Arch Linux / Wayland / KWin, NVIDIA 610.43.03, Vulkan 1.4.357:
- all 304 build targets compile
- 22 chapters that present a frame: launched and closed with a real
compositor close request, all exit 0 (each exited 139 before)
- 14_command_buffers (never presents, so its surface is never mapped and
it cannot be closed by the compositor) checked by breaking out of the
main loop instead: 3/3 SIGSEGV before, 3/3 clean after
- no validation layer errors, and no coredumps from any fixed binary
Refs: KhronosGroup#138
|
On windows, I don't have any issues with window closing, here. But shouldn't the GlfwGuard have a constructor like this: And even though implicit destruction of the GLFWwindow works fine, maybe should then introduce some wrapper for it as well, like Or, as already mentioned in #138, switch to vkfw? Besides that, I would not keep an empty |
Follow-up to review feedback on KhronosGroup#443. `glfwInit()` was called at the top of `initWindow()` and its return value discarded in every chapter, so a failed initialization surfaced later as a null window rather than as an error. Moving it into `GlfwGuard` gives the guard both ends of the GLFW lifetime and gives the failure somewhere to go: GlfwGuard() { if (!glfwInit()) { throw std::runtime_error("failed to initialize GLFW!"); } } The constructor runs during member initialization, before `run()` is entered, and every chapter already constructs the application object inside the `try` in `main()`, so the error is reported through the same path as every other `std::runtime_error`. If it throws, `~GlfwGuard()` never runs, so `glfwTerminate()` is correctly skipped after a failed init. Also in this change: - 34_android.cpp gains the same guard. Its PLATFORM_DESKTOP path creates a GLFW window but never called `glfwTerminate()` or `glfwDestroyWindow()`, so it leaked GLFW rather than destroying it out of order. Every chapter that owns a window now uses the guard. - `cleanup()` was left as bare empty braces in 31_compute_shader, 32_ecosystem_utilities, 35_gltf_ktx and 38_ray_tracing, while 28 other chapters carried a comment explaining where GLFW teardown had moved to. Those four now match. - The guard in 32_ecosystem_utilities moved above `AppInfo appInfo` so that "declared first" is literally true there. - 00_Base_code.adoc introduces the guard where GLFW initialization is taught, instead of reintroducing it in the cleanup section, and 04_Swap_chain_recreation.adoc drops `glfwInit()` from its initWindow snippet. Verified on Windows 11, MSVC 14.51, Vulkan 1.4.357, RTX 5080: - all 144 build targets compile with no new warnings - clang-format-diff.py over the diff against main reports no violations - every chapter built from a pristine main and from this branch, then closed with a real WM_CLOSE and its exit code recorded: 36/36 exit 0 on both sides. Windows does not fault on the original ordering, so this measures no regression rather than a fix on that platform. Refs: KhronosGroup#138 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Hi, I compiled and ran the tutorial files on a Linux/Wayland desktop and the crash happens on every desktop chapter there. On Windows it quits without crashing, like you say. But the ordering still looks wrong to me on both, it just doesn't blow up on Win32. I checked Windows here as well before answering, so it's not only guesswork. I built every chapter from a clean main and from this branch, ran them all and closed each window the normal way. Nothing crashed on either side and I couldn't see any regression from the change. I'm still learning Vulkan, so I don't want to overstate the Wayland part. What I could follow from the backtrace is that What I wanted with this PR was to fix that without pulling in a new dependency or rewriting too much of the tutorial code. About struct GlfwGuard
{
GlfwGuard()
{
if (!glfwInit())
{
throw std::runtime_error("failed to initialize GLFW!");
}
}
~GlfwGuard()
{
glfwTerminate();
}
} glfwGuard;
The None of that is an objection in principle. It just makes this a refactor of the tutorial's window setup rather than a crash fix, and it doesn't change teardown behaviour, since Same for vkfw, only more so, since that is a new dependency in every chapter plus all the prose around it. I'd have thought that one belongs in #138 rather than here. I also ran a code review with Claude Code over the rest of the chapters. Its notes are below. Claude Code review notes
Inconsistent
Verification. All 144 targets build clean under MSVC 14.51 with no new warnings. The repository's own |
|
The The job fails while applying the Android Gradle plugin, before any C++ gets compiled:
This PR seems to be the first one to actually run the job. For completeness, the change can't affect the Android build. In all three Android-supported chapters it touches ( Two ways to get the job green again, either as a one-liner here or as a separate PR:
|
Follow-up to review feedback on KhronosGroup#443. `glfwInit()` was called at the top of `initWindow()` and its return value discarded in every chapter, so a failed initialization surfaced later as a null window rather than as an error. Moving it into `GlfwGuard` gives the guard both ends of the GLFW lifetime and gives the failure somewhere to go: GlfwGuard() { if (!glfwInit()) { throw std::runtime_error("failed to initialize GLFW!"); } } The constructor runs during member initialization, before `run()` is entered, and every chapter already constructs the application object inside the `try` in `main()`, so the error is reported through the same path as every other `std::runtime_error`. If it throws, `~GlfwGuard()` never runs, so `glfwTerminate()` is correctly skipped after a failed init. Also in this change: - 34_android.cpp gains the same guard. Its PLATFORM_DESKTOP path creates a GLFW window but never called `glfwTerminate()` or `glfwDestroyWindow()`, so it leaked GLFW rather than destroying it out of order. Every chapter that owns a window now uses the guard. - `cleanup()` was left as bare empty braces in 31_compute_shader, 32_ecosystem_utilities, 35_gltf_ktx and 38_ray_tracing, while 28 other chapters carried a comment explaining where GLFW teardown had moved to. Those four now match. - The guard in 32_ecosystem_utilities moved above `AppInfo appInfo` so that "declared first" is literally true there. - 00_Base_code.adoc introduces the guard where GLFW initialization is taught, instead of reintroducing it in the cleanup section, and 04_Swap_chain_recreation.adoc drops `glfwInit()` from its initWindow snippet. Verified on Windows 11, MSVC 14.51, Vulkan 1.4.357, RTX 5080: - all 144 build targets compile with no new warnings - clang-format-diff.py over the diff against main reports no violations - every chapter built from a pristine main and from this branch, then closed with a real WM_CLOSE and its exit code recorded: 36/36 exit 0 on both sides. Windows does not fault on the original ordering, so this measures no regression rather than a fix on that platform. Refs: KhronosGroup#138
207324c to
8d7abf5
Compare
|
We are aware of the Android issue, it also occurs in one of our own PRs. |
|
It's a pretty elegant solution. I tested it on Wayland, and Win, works for me. And thanks for updating the prose in the tutorial. I'm happy with this. |
|
@gpx1000 Thanks a lot, I’m really glad it works well on both Wayland and Windows. I appreciate you taking the time to test it and review the tutorial updates. |
|
FYI, Android CI now has a PR fix available: #458 When it merges to main, merge to the latest and we will take this PR. Thanks for the work. |
|
@gpx1000 Just a quick follow-up: #458 has landed on No conflicts, and nothing outstanding from the review — this should be ready whenever you have a moment. Thanks again to both of you for the reviews. |
|
We discussed this on a recent meeting and while I'm fine with how this works, having this fix duplicated almost 40 times is something I'm not to keen on merging. I'd prefer having the GlfwGuard in a separate header file that's included in the chapters. That would remove code-duplication, something the tutorial already suffers from and something we want to address in the future anyway. |
|
Hi @ozanyasindogan, can you reply to Sascha's comment above? Any way to adjust this to accommodate his request? Thanks. |
Hello everyone, Thank you for following up, and thank you, Sascha, for the suggestion. Sorry for the delayed response. I initially misunderstood your comment. I thought you meant that this fix would be handled as part of a broader effort to consolidate and reorganize the tutorial code, and that you didn’t intend to proceed with this PR. That’s why I hadn’t followed up further. My original intention was to fix the crashes I encountered on Arch Linux with Wayland while keeping the changes small and preserving the tutorial’s existing structure. Moving Since some time has passed, I’ve also checked the latest Would you like me to bring this PR up to date with Thank you again for reviewing and testing the fix. |
Yes, that would be ideal and reduce code duplications. |
The guard was repeated in all 36 chapters that own a window. It now lives once in attachments/glfw_guard.h, together with the explanation of why it has to be declared first. Each chapter includes the header right after its own GLFW include, so GLFW_INCLUDE_VULKAN is still defined when GLFW is first included, and declares `GlfwGuard glfwGuard;` as its first member with a one-line comment, since that ordering is what keeps GLFW alive until every vk::raii member is gone. In 34_android, 35_gltf_ktx and 36_multiple_objects both stay inside the existing desktop-only branch. Docs: - Base code teaches the header, the include order and the member. - Every page that links a chapter's source now also links glfw_guard.h, so a chapter downloaded on its own still compiles. - The ray tracing course overview lists the header with the other files. - The Multiple Objects cleanup() snippet no longer calls glfwDestroyWindow()/glfwTerminate(). It was added after this PR was opened and showed exactly the ordering this PR fixes; the chapter's code already doesn't do it.
|
@SaschaWillems I've updated the PR as you suggested. On Arch Linux with KDE Plasma on Wayland, There's one open question for you at the end of the description, about the license annotation for the new header. Also, the four workflow runs are waiting for maintainer approval. Could someone approve them when convenient? @gpx1000 @asuessenbach the behaviour is the same as the version you approved, but the structure has changed, so I'd appreciate another look. |
Fixes the crash on window close reported in #138.
Problem
On the tested Arch Linux/Wayland setup, the affected desktop chapters crashed during shutdown because GLFW was terminated before the Vulkan objects were destroyed.
run()ends withcleanup(), which calledglfwDestroyWindow()andglfwTerminate(). The Vulkan objects arevk::raiiclass members, so they're only destroyed afterrun()returns, by which time GLFW has already closed the connection to the window system. Destroying the swapchain then crashes insidelibwayland-client(SIGSEGV from~SwapchainKHR). Before the move tovk::raii,cleanup()destroyed the Vulkan objects by hand and calledglfwTerminate()last; the conversion removed those calls but left the GLFW ones, which reversed the order.Fix
A new shared header,
attachments/glfw_guard.h, definesGlfwGuard. Its constructor callsglfwInit()and throws if that fails; its destructor callsglfwTerminate(). Each chapter declares the guard as its first member:Members are destroyed in reverse declaration order, so GLFW is now terminated after every Vulkan object, however many members later chapters add.
glfwTerminate()also destroys any windows that are still open, so the separateglfwDestroyWindow()call is gone.The header is included after the chapter's own
#include <GLFW/glfw3.h>, because it includes GLFW itself and would otherwise pull it in beforeGLFW_INCLUDE_VULKANis defined. A failedglfwInit(), which was previously ignored, is now reported at startup through the existingtry/catchinmain().Changes
attachments/glfw_guard.h: new. No CMake changes are needed.00_base_code.cppto38_ray_tracing.cpp): include the header and declare the guard first.glfwInit()is removed frominitWindow(), and the GLFW teardown is removed fromcleanup(). In34_android.cpp,35_gltf_ktx.cppand36_multiple_objects.cpp, all of this is inside the existing desktop-only branch.00_Base_code.adocexplains the header, the include order, and why the guard is declared first.cleanup()snippets in04_Swap_chain_recreation.adocand16_Multiple_Objects.adocno longer callglfwDestroyWindow()/glfwTerminate()..cppalso linksglfw_guard.h, so a chapter downloaded on its own still compiles. The ray tracing course overview lists the header with its other files.Testing
The current tests compare this PR with
54b886e, the upstream commit merged into this branch. Linux tests ran on the published heade5c9901. Windows and Android tests ran on1e639b8, whose Git tree is identical toe5c9901; those tests were not rerun after the commit messages changed. All window-close tests were scripted. Earlier results are listed separately below.54b886e14_command_buffers×3 and30_multisampling×5glfwInit()failsWM_CLOSE54b886e(also with-Wall -Wextraand/W4); none fromglfw_guard.h-DENABLE_CPP20_MODULE=ON): Linux GCC and Clang, Windows MSVCNotes on these results:
54b886ehas the same stack:~SwapchainKHRin the application's destructor, then the NVIDIA driver andwl_proxy_marshal_flags, then the crash inlibwayland-client. The Linux close script starts each chapter, waits 6 seconds, then has KWin callcloseWindow()on that process's window, which sends the same close request as the title-bar X.33_vulkan_profilesreported anything:VUID-vkCmdPipelineBarrier2-synchronization2-03848, while recording frames, identically on both revisions. It's an existing issue in that chapter and not related to shutdown.glfw_guard.hor GLFW at all. I couldn't test Vulkan teardown on Android. On an emulator, every APK from both revisions crashes inGameActivity.onCreatebecause the activity's theme isn't an AppCompat theme. With the theme changed locally, the x86_64 APKs then fail with anUnsatisfiedLinkErrorforGameActivity.initializeNativeCode. Both problems exist upstream and are unrelated to this PR.vulkan.cppmimports thestdmodule, which the build doesn't provide, and the module is now namedvulkanwhile the chaptersimport vulkan_hpp. CI doesn't build this configuration. To check the include order for it anyway, I preprocessed every chapter withUSE_CPP20_MODULES=1:glfwCreateWindowSurfaceis declared in the same chapters on both revisions.make setupinantora/runs without errors on both revisions. On this branch it copiesglfw_guard.h, and 39 generated pages link to it.simple_engine/*.cpptargets are missing on both revisions; that's an existing upstream problem.Open question
glfw_guard.hcurrently inherits Apache-2.0 from theattachments/**rule inREUSE.toml, while many of the early chapter sources it's taken from are listed as CC0-1.0. Would you prefer an explicit CC0 annotation for the header, so those examples stay consistent? I've leftREUSE.tomlunchanged for now.Earlier results (before the shared header, against
e8c3ba2)These are from earlier revisions of this PR, when the guard was still defined inside each chapter. The baseline is
e8c3ba2, the upstream commit the branch was based on at the time.55de73a, Linux/Wayland: Arch Linux, KWin 6.7.3, NVIDIA 610.43.03, Vulkan 1.4.357, GLFW 3.5.1, GCC 16.1.1. The same 22 chapters were closed through KWin by a script. One8c3ba2all 22 crashed with SIGSEGV; on55de73aall 22 exited with 0.14_command_buffers(3 runs) and30_multisampling(5 runs) were run with the main loop ended after a fixed number of iterations: they crashed every time one8c3ba2and exited cleanly every time on55de73a. As a control, removing only theglfwTerminate()call frome8c3ba2also stopped the crash (5/5). That control leaks the connection, though, while this PR destroys it in the right order.8d7abf5, Windows: all 36 chapters closed viaWM_CLOSEexited with 0 on bothe8c3ba2and8d7abf5.