Skip to content

Handle lazy IO property allocation failures - #16382

Merged
slouken merged 1 commit into
libsdl-org:mainfrom
alistanis:ccooper/sdl-io-properties-oom
Sep 24, 2026
Merged

slouken merged 1 commit into
libsdl-org:mainfrom
alistanis:ccooper/sdl-io-properties-oom

Conversation

@alistanis

@alistanis alistanis commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor
  • I confirm that I am the author of this code and release it to the SDL project under the zlib license. This contribution does not contain code from other sources, including code generated by a Large Language Model ("AI").

Only publish stream property groups after all initializer calls succeed. Destroy incomplete groups and allow retry so failed property insertion cannot orphan a dynamic stream's existing buffer.

Add an isolated allocation failure regression covering close, retry, and explicit ownership transfer.

Description

This change prevents failed property initialization from publishing incomplete properties and leaking a stream’s buffer, that's pretty much it.

Existing Issue(s)

None, but can create one if necessary.

Thanks,
Chris

@slouken

slouken commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

This change seems fine to me, although have you seen our AI policy?
https://github.com/libsdl-org/SDL/blob/main/AGENTS.md

Please remove the test program and go ahead and add additional tests to test/testautomation_iostream.c if they are warranted.

@slouken
slouken requested a review from icculus September 24, 2026 17:44
@slouken slouken added this to the 3.6.0 milestone Sep 24, 2026
@icculus

icculus commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Yeah, that test program is...pretty aggressive.

Emphasis on "if they are warranted," which they likely are not for this PR.

@alistanis

alistanis commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor Author

Yes I've seen the policy; I work at Nasuni as a Principal SWE and these are the kinds of tests we write for our filesystem. I work on the backend data path team and we try to be very thorough; I have no problem dialing it back

@alistanis
alistanis force-pushed the ccooper/sdl-io-properties-oom branch from 31ae2f9 to 2f9f6ca Compare September 24, 2026 18:00
@alistanis

Copy link
Copy Markdown
Contributor Author

Removed the tests, thanks for the feedback; I'll try not to be so aggressive next time or I'll draft and reach out asking what you'd like.

@alistanis

Copy link
Copy Markdown
Contributor Author

Forgot to remove the CMake entry...

Only publish stream property groups after all initializer calls succeed.
Destroy incomplete groups and allow retry so failed property insertion
cannot orphan a dynamic stream's existing buffer.

Add an isolated allocation failure regression covering close, retry, and
explicit ownership transfer.
@alistanis
alistanis force-pushed the ccooper/sdl-io-properties-oom branch from 2f9f6ca to 6475037 Compare September 24, 2026 18:05
@alistanis

Copy link
Copy Markdown
Contributor Author

Rebased and removed the CMakeLists entry; please let me know if you'd like anything else.

Comment thread src/io/SDL_iostream.c
@slouken
slouken merged commit 7f3d063 into libsdl-org:main Sep 24, 2026
10 checks passed
@slouken

slouken commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

Merged, thanks!

@alistanis

Copy link
Copy Markdown
Contributor Author

Thanks Sam, and for what it's worth, thank you for all you've done. I've been working with SDL for a while on my personal projects, and I really appreciate it.

@slouken

slouken commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

You're welcome! :)

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