Skip to content

Fix bug in event.cpp where queue_impl outlives async handler and causes a segmentation fault#1174

Merged
bader merged 5 commits intoKhronosGroup:mainfrom
lbushi25:wait_and_throw_fail
Jan 29, 2026
Merged

Fix bug in event.cpp where queue_impl outlives async handler and causes a segmentation fault#1174
bader merged 5 commits intoKhronosGroup:mainfrom
lbushi25:wait_and_throw_fail

Conversation

@lbushi25
Copy link
Copy Markdown
Contributor

@lbushi25 lbushi25 commented Jan 26, 2026

This PR fixes a flaky bug in event.cpp where an async exception handler associated to a queue had already gone out of scope by the time the queue_impl destructor was called. Since the latter(destructor) calls the former(exception handler), this is clearly a problem as we get undefined behavior. This PR moves the exception handler, previously declared in function scope, to global scope so that it always outlives the queue resources.

@lbushi25 lbushi25 requested a review from a team as a code owner January 26, 2026 20:09
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jan 26, 2026

CLA assistant check
All committers have signed the CLA.

@lbushi25 lbushi25 changed the title [SYCL] Fix bug where queue_impl outlives async handler and causes a segmentation fault Fix bug in event.cpp where queue_impl outlives async handler and causes a segmentation fault Jan 26, 2026
@TApplencourt
Copy link
Copy Markdown
Contributor

TApplencourt commented Jan 27, 2026

Haha, related too the recent KhronosGroup/SYCL-Docs#925
We will discuss in the next call, just to be sure everybody have the same understanding

@TApplencourt TApplencourt added the Agenda To be discussed during a SYCL committee meeting label Jan 27, 2026
@TApplencourt
Copy link
Copy Markdown
Contributor

In theory better to clear(); at the beginning of the test, not at the end.

Indeed if a test fail, maybe clear() will not be triggered and leave the handle in a bad state. But 🤷🏽 , not that critical.
Thanks for the fix!

@bader
Copy link
Copy Markdown
Contributor

bader commented Jan 29, 2026

In theory better to clear(); at the beginning of the test, not at the end.

Indeed if a test fail, maybe clear() will not be triggered and leave the handle in a bad state. But 🤷🏽 , not that critical. Thanks for the fix!

@lbushi25, let me know if you are going to apply this comment.

@TApplencourt, once @lbushi25 confirms that PR is ready, I can merge it, right?

@lbushi25
Copy link
Copy Markdown
Contributor Author

lbushi25 commented Jan 29, 2026

In theory better to clear(); at the beginning of the test, not at the end.
Indeed if a test fail, maybe clear() will not be triggered and leave the handle in a bad state. But 🤷🏽 , not that critical. Thanks for the fix!

@lbushi25, let me know if you are going to apply this comment.

@TApplencourt, once @lbushi25 confirms that PR is ready, I can merge it, right?

I made the changes. As for me, once I check that the CI passes, we can merge it.

@lbushi25
Copy link
Copy Markdown
Contributor Author

lbushi25 commented Jan 29, 2026

@bader Feel free to merge if @TApplencourt agrees!

@bader bader merged commit 9c1df79 into KhronosGroup:main Jan 29, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Agenda To be discussed during a SYCL committee meeting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants