Skip to content

Make SdlDrop and SubsystemDrop safer to use #1254

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

Merged
merged 2 commits into from
Sep 7, 2022
Merged

Make SdlDrop and SubsystemDrop safer to use #1254

merged 2 commits into from
Sep 7, 2022

Conversation

daggerbot
Copy link
Contributor

@daggerbot daggerbot commented Aug 31, 2022

Fixes #1247 and #1248. Ensures better initialization/drop guarantees for SdlDrop and SubsystemDrop by using global reference counts. This has a few caveats:

  • SdlDrop can no longer be initialized from outside the crate::sdl module. Any external code depending on this (hopefully there was none because this is extremely unsafe) will break.
  • Public but hidden functions returning SdlDrop and SubsystemDrop are no longer wrapped in Rc<>. Any external code depending on these functions will break. It would be possible to reverse this change, but with global reference counting, there is no longer any good reason to do so.
  • The fixes caused at least one of the tests to break because tests are run in parallel by default, and more than one test initializes SDL. Now that this can only be done from one thread, one must specify --test-threads=1 for the tests to pass. Maybe this was already the case, but I didn't check before making these changes. Example: cargo test -- --test-threads=1

Because the first two caveats are API breaks, even if hidden from documentation, I recommend a version bump to 0.36.0 if these changes are accepted.

@Cobrand
Copy link
Member

Cobrand commented Sep 1, 2022

Nice job!

The fixes caused at least one of the tests to break because tests are run in parallel by default, and more than one test initializes SDL. Now that this can only be done from one thread, one must specify --test-threads=1 for the tests to pass. Maybe this was already the case, but I didn't check before making these changes. Example: cargo test -- --test-threads=1

This was already done in Github Actions for the tests, so no worries about that.

Could you add a line in the changelog to mention your changes? It's internal but who knows, it might break someone's code.

@daggerbot
Copy link
Contributor Author

Could you add a line in the changelog to mention your changes? It's internal but who knows, it might break someone's code.

Done. Do you want the changelog addition squashed, or is it fine like this?

@Cobrand
Copy link
Member

Cobrand commented Sep 7, 2022

Thanks!

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.

Publicly exporting SdlDrop is unsafe
2 participants