Skip to content
New issue

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

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

Already on GitHub? # to your account

Make bindings resilient to OOM errors #923

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tomaka
Copy link

@tomaka tomaka commented Jun 8, 2024

This PR proposes a relatively simple but maybe controversial change.

In the functions that return a Vec, instead of using Vec::with_capacity and similar, we use Vec::new() followed with Vec::try_reserve. If try_reserve fails, we return VK_ERROR_OUT_OF_HOST_MEMORY.

Note that there's no shorter way on stable Rust to do this. Notably, try_with_capacity is unstable.

If I'm not mistaken, the bindings now never panic in case of OOM.

I personally don't have a need for this change, but I thought that this was a neat thing to add.

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how useful this is in practice, but it seems like a strict improvement, and not too bad to maintain.

@MarijnS95
Copy link
Collaborator

If we end up doing this, is there a lint that enables us to catch accidental/new remaining calls to Vec::with_capacity() and friends, that may result in panicking allocations?

Alternatively we could list such functions in disallowed-methods in clippy.toml via clippy::disallowed_methods.

I personally don't have a need for this change, but I thought that this was a neat thing to add.

Might be neat, but if we do this we're better complete about it :)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants