Skip to content
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

Allow the crate to operate when the Redis API isn't available. #300

Merged

Conversation

iddm
Copy link
Collaborator

@iddm iddm commented Mar 28, 2023

Such situations occur when, for example, the crate which uses the redismodule-rs crate as a dependency is run for testing, so without Redis available. In this case, there is no allocator available as well, and it will lead to panic. To enable both situations to work correctly under all circumstances, this change introduces a fallback mechanism back to the system allocator (default Rust allocator), which is always available.

Note that the changes are zero-cost: the code before these changes used unwrap(), which also had checks for the value being non-null and would panic at runtime if it was null; the current code does this check as well, but instead of panicking reverts back to the system allocator.

@iddm iddm requested a review from gkorland March 28, 2023 10:41
@iddm iddm self-assigned this Mar 28, 2023
@iddm
Copy link
Collaborator Author

iddm commented Mar 28, 2023

Without these changes, it is currently impossible for any other crate depending on this one (redismodule-rs) to run tests and work without Redis.

iddm added 2 commits March 28, 2023 13:14
Such situations occur when, for example, the crate which uses
the redismodule-rs crate as a dependency is run for testing, so
without Redis available. In this case, there is no allocator available
as well and it will lead to a panic. To enable both the situations to
work correctly under all circumstances, this change introduces a
fallback mechanism back to the system allocator (default Rust allocator)
which is always available.

Note that the changes are zero-cost: the code before this changes used
unwrap() which also had checks for the value being non-null and would
panic at runtime if it was null, the current code does this check as
well but instead of panicking reverts back to the system allocator.
@iddm iddm force-pushed the fallback-to-the-system-allocator-when-redis-isnt-available branch from cc46c7e to cf0473c Compare March 28, 2023 11:14
@iddm iddm force-pushed the fallback-to-the-system-allocator-when-redis-isnt-available branch from 4242cf6 to f873f8a Compare March 28, 2023 11:17
@iddm
Copy link
Collaborator Author

iddm commented Mar 28, 2023

Also fixes: #89

The previous code either wouldn't compile or wouldn't run due to missing
assets for the tests. This commit brings changes necessary to enable the
cargo test to run properly again.
@iddm
Copy link
Collaborator Author

iddm commented Mar 28, 2023

I also highly recommend running the cargo test (without the makefile and additional arguments) in CI.

@iddm iddm requested a review from MeirShpilraien March 29, 2023 10:36
src/alloc.rs Outdated Show resolved Hide resolved
src/alloc.rs Show resolved Hide resolved
@iddm iddm force-pushed the fallback-to-the-system-allocator-when-redis-isnt-available branch from 0c58923 to 90770dc Compare March 29, 2023 14:01
@iddm iddm requested a review from oshadmi March 29, 2023 14:22
Reverts the behaviour to panicking when the redis allocator isn't
present. Changes the panic to avoid allocations using the specified
allocator so that a meaningful message can be observed.
@iddm iddm force-pushed the fallback-to-the-system-allocator-when-redis-isnt-available branch from 90770dc to 03b52b9 Compare March 29, 2023 14:29
@MeirShpilraien
Copy link
Collaborator

@vityafx @oshadmi this pr is made for api_extantions branch. We are gone drop this branch soon as all the changes was added to master. For progress lets merge this and make sure to take the relevant changes to master on another PR.

@iddm iddm merged commit 1e735bd into api_extentions Mar 29, 2023
@iddm iddm deleted the fallback-to-the-system-allocator-when-redis-isnt-available branch March 29, 2023 17:33
@BruAPAHE
Copy link

@vityafx @gkorland I get Critical error: the Redis Allocator isn't available panic when i run tests cargo test. Anybody resolve this problem?

@MeirShpilraien
Copy link
Collaborator

Hey @BruAPAHE , look how we solve it on RedisGears: https://github.com/RedisGears/RedisGears/blob/c6de5d50eb92ecbdfc976fb5b15bd6beb9d44c12/redisgears_core/src/lib.rs#L1256

@BruAPAHE
Copy link

@MeirShpilraien Thank you so much!!!

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.

5 participants