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

fix/db_creation_runtime_crash #214

Merged

Conversation

Charles-Schleich
Copy link
Member

…ll_entries, get and deletion exhibiting similar behaviour

…ll_entries, get and deletion exhibiting similar behaviour
Copy link

github-actions bot commented Sep 9, 2024

PR missing one of the required labels: {'dependencies', 'bug', 'internal', 'breaking-change', 'new feature', 'enhancement', 'documentation'}

@Charles-Schleich Charles-Schleich added the bug Something isn't working label Sep 9, 2024
Copy link
Contributor

@J-Loudet J-Loudet left a comment

Choose a reason for hiding this comment

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

I am not comfortable with the Tokio executor management so, from my limited understanding, I don't see any issue 👍
I will let @evshary approve

@oteffahi
Copy link
Contributor

oteffahi commented Sep 10, 2024

a DELETE message seems to still trigger a runtime error.

thread '<unnamed>' panicked at ~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-0.14.28/src/client/connect/dns.rs:121:24:
there is no reactor running, must be called from the context of a Tokio 1.x runtime
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
fatal runtime error: Rust cannot catch foreign exceptions

Copy link
Contributor

@evshary evshary left a comment

Choose a reason for hiding this comment

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

I think we might have a more consistent to decide when to use await_task.
The reason we need await_task is that the async fn (put, del...) inside the impl trait (like Volume / Storage) can't await the async task directly. They can't get the current Runtime and need to use the Runtime inside plugins.
Therefore the better way here is that we only apply await_task to the function related to the trait Volume and Storage. Besides, we still use the normal await.

For example,
We don't need to use await_task inside get_deletion_timestamp, but use it here.

if let Some(del_time) = self.get_deletion_timestamp(measurement.as_str()).await? {

We can ensure consistency in this way, and developers can easily know what we're doing.

BTW, just as @oteffahi reported, there are some missing parts in put and del

if let Some(del_time) = self.get_deletion_timestamp(measurement.as_str()).await? {


if let Err(e) = self

if let Err(e) = self

Now I'm thinking of moving blockon_runtime and await_task to Zenoh and then every plugin does not need to define them by itself.
But it's beyond the PR.

v2/src/lib.rs Outdated Show resolved Hide resolved
v2/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@evshary evshary left a comment

Choose a reason for hiding this comment

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

LGTM
I can also run the test successfully, but I'll leave @oteffahi to do the final test since I couldn't reproduce the DEL panic.

@Charles-Schleich Charles-Schleich changed the title blockon dynamic plugin loading cause db creation runtime crash. get_a… fix/db_creation_runtime_crash Sep 12, 2024
Copy link
Contributor

@oteffahi oteffahi left a comment

Choose a reason for hiding this comment

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

LGTM. Other unrelated issues were found. Will address them in another PR.

@gabrik gabrik merged commit d18ba52 into eclipse-zenoh:main Sep 12, 2024
6 checks passed
@gabrik gabrik deleted the fix/db_creation_runtime_crash branch September 12, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants