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

[Feat] run func timeout use std::time::Duration #76

Merged
merged 2 commits into from
Oct 2, 2023

Conversation

L-jasmine
Copy link
Collaborator

No description provided.

Copy link
Member

juntao commented Sep 27, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Overall, the pull request introduces several changes related to handling timeouts and asynchronous operations. The changes include replacing the timeout parameter with the deadline parameter of type std::time::SystemTime in the TimeoutFiberFuture struct, and replacing the timeout parameter with the timeout parameter of type std::time::Duration in various functions of the Executor, Func, and Vm structs.

The potential problems in the code include handling errors when calculating the timeout duration, assuming the timeout value is already in seconds and casting it to i64 without validation, setting a minimum timeout of 100 milliseconds in the TimeoutFiberFuture struct, not handling errors when setting the timer, potential duplication of similar functionality in different structs, and lack of documentation explaining the use of the deadline parameter in the TimeoutFiberFuture struct.

Additionally, the pull request includes updates to the API documentation and introduces a new async_state argument to multiple functions. However, there is a lack of context and explanation for these changes, requiring further documentation or code comments to clarify their purpose.

In conclusion, the pull request addresses timeout handling and asynchronous operations, but it lacks necessary error handling, validation, and documentation to ensure the reliability and understandability of the code.

Details

Commit 1b0bf586540c17e79fc96a07562ab9fb10bdae40

Key changes in this PR:

  • The timeout parameter in the on_fiber function of the TimeoutFiberFuture struct has been replaced with the deadline parameter of type std::time::SystemTime.
  • The timeout parameter in the call_func function of the Executor struct has been replaced with the timeout parameter of type std::time::Duration.
  • The timeout parameter in the call_func_with_timeout function of the Executor struct has been replaced with the timeout parameter of type std::time::Duration.
  • The timeout parameter in the call_func_async_with_timeout function of the Executor struct has been replaced with the timeout parameter of type std::time::Duration.
  • The timeout parameter in the run_func_with_timeout function of the Func struct has been replaced with the timeout parameter of type std::time::Duration.
  • The timeout parameter in the run_func_async_with_timeout function of the Func struct has been replaced with the timeout parameter of type std::time::Duration.
  • The timeout parameter in the run_func function of the Vm struct has been replaced with the timeout parameter of type std::time::Duration.

Other changes include updating the code to use the new deadline and timeout parameters.

Potential problems:

  1. The code does not handle errors that may occur when calculating the timeout duration.
  2. The timeout value is expected to be in seconds, but the code assumes it is already in seconds and directly casts it to i64 and assigns it to value.it_value.tv_sec in the call_func_with_timeout and call_func_async_with_timeout functions of the Executor struct. This may cause unexpected behavior if the timeout value is not in seconds.
  3. In the TimeoutFiberFuture struct, the timeout value is used for setting the timespec value of the timer. However, the code sets a minimum timeout of 100 milliseconds using timeout.max(std::time::Duration::from_millis(100)). This could cause the timer to fire prematurely if the original timeout value is less than 100 milliseconds.
  4. The code does not handle potential errors that may occur when setting the timer using libc::timer_settime. If an error occurs, it returns an Err(()) without providing any information about the error that occurred.
  5. There seem to be duplicate implementations of similar functionality in the code (e.g., call_func and call_func_with_timeout functions in both the Executor and Vm structs). It may be worth reviewing the code to check if these implementations can be consolidated or if there is any overlap in functionality.
  6. The code does not provide any documentation or comments explaining the reasoning behind using the deadline parameter in the TimeoutFiberFuture struct instead of the timeout_sec parameter. It would be helpful to have some explanation to understand the intention behind this change.

Commit df343d91f73b4ed5341e46f9ed085cd101c74662

Key changes:

  • Updated the API documentation in multiple files.
  • Added a new argument async_state to various functions.

Potential problems:

  • It is unclear from the patch description why the API documentation was updated. It would be helpful to have more context on the changes made to the documentation.
  • The patch introduces a new async_state argument to multiple functions, but it is not clear how it is being used or what its purpose is. It may be necessary to document this addition more thoroughly or provide more context in the code comments.

Overall, the changes seem to be focused on adding support for asynchronous operations and updating the API documentation. However, more information is needed to fully understand the changes and their implications.

crates/wasmedge-sys/src/executor.rs Show resolved Hide resolved
@@ -115,15 +115,15 @@ impl<'a> TimeoutFiberFuture<'a> {
///
/// * `func` - The function to execute.
///
/// * `timeout_sec` - The maximum execution time in seconds for the function instance.
/// * `deadline` - The deadline the function instance.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please refine the description for the deadline argument. Thanks a lot!

crates/wasmedge-sys/src/async/fiber.rs Outdated Show resolved Hide resolved
crates/wasmedge-sys/src/async/fiber.rs Show resolved Hide resolved
@apepkuss apepkuss self-requested a review October 2, 2023 03:18
@apepkuss apepkuss merged commit 07e84b7 into WasmEdge:main Oct 2, 2023
L-jasmine added a commit to second-state/wasmedge-rust-sdk that referenced this pull request Nov 1, 2023
…un_func_async_with_timeout` (WasmEdge#76)

* [Feat] run func timeout use std::time::Duration

Signed-off-by: csh <[email protected]>

* [Doc]: update api docs

Signed-off-by: csh <[email protected]>

---------

Signed-off-by: csh <[email protected]>
(cherry picked from commit 07e84b7)
L-jasmine added a commit to second-state/wasmedge-rust-sdk that referenced this pull request Nov 1, 2023
…un_func_async_with_timeout` (WasmEdge#76)

* [Feat] run func timeout use std::time::Duration

Signed-off-by: csh <[email protected]>

* [Doc]: update api docs

Signed-off-by: csh <[email protected]>

---------

Signed-off-by: csh <[email protected]>
(cherry picked from commit 07e84b7)
Signed-off-by: csh <[email protected]>
@juntao juntao mentioned this pull request Nov 8, 2023
apepkuss added a commit that referenced this pull request Nov 28, 2023
* [Refactor] Add #[form] for subtypes of WasmEdgeError.

Signed-off-by: csh <[email protected]>

* [Refactor] fix memory leak

Signed-off-by: csh <[email protected]>

* [Fix] Avoid the CPU 100% by tokio-wasi.

Signed-off-by: csh <[email protected]>

* [Fix] Fix blocking during tokio-wasi TCP connect.

Signed-off-by: csh <[email protected]>

* [Fix] async-wasi get envs & args

Signed-off-by: csh <[email protected]>

* [Perf] Optimize the wasi-ctx

Signed-off-by: csh <[email protected]>

* [Chore] Temporarily disable the serialize feature.

Signed-off-by: csh <[email protected]>

* [feat] virtual file system

Signed-off-by: csh <[email protected]>

* [Refactor] update the argument type of `run_func_with_timeout` and `run_func_async_with_timeout` (#76)

* [Feat] run func timeout use std::time::Duration

Signed-off-by: csh <[email protected]>

* [Doc]: update api docs

Signed-off-by: csh <[email protected]>

---------

Signed-off-by: csh <[email protected]>
(cherry picked from commit 07e84b7)
Signed-off-by: csh <[email protected]>

* [Refactor] delete useless code from validator

Signed-off-by: csh <[email protected]>

* [Refactor] To pass the clippy

Signed-off-by: csh <[email protected]>

* Disable timeout in musl libc (#71)

Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: csh <[email protected]>

* Disable timeout in musl libc

Signed-off-by: csh <[email protected]>

* [Doc] update doc

Signed-off-by: csh <[email protected]>

* [Fix] fix ffi::WasmEdge_String to String

Signed-off-by: csh <[email protected]>

* [Fix] Fix unit test

Signed-off-by: csh <[email protected]>

* [Fix] Modify the `WasiModule`.

Signed-off-by: csh <[email protected]>

* [refactor] Introduce `WasmEdge_FunctionInstanceGetData` to drop host data (#84)

* feat(rust-sys): drop host_data in `Function::drop`

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update `drop` of `ImportModule` and `Function`

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sdk): update test code

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update `drop` of `ImportModule` and `Function`

Signed-off-by: Xin Liu <[email protected]>

* refactor(rust-sys): update `ImportModule::drop`

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): remove debug code

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sdk): remove debug code

Signed-off-by: Xin Liu <[email protected]>

* version(rust-sdk): bump to `0.13.0`

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): supress clippy warning

Signed-off-by: Xin Liu <[email protected]>

* ci(ci-build): update rust version

Signed-off-by: Xin Liu <[email protected]>

* ci(standalone): update rust version

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update rustdoc

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sdk): update rustdoc and `README`

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update dependencies

Signed-off-by: Xin Liu <[email protected]>

* ci: update to `macos-13`

Signed-off-by: Xin Liu <[email protected]>

---------

Signed-off-by: Xin Liu <[email protected]>

* doc: update `CHANGELOG` (#85)

Signed-off-by: Xin Liu <[email protected]>

* [Chore] Update build script (#86)

* chore(rust-sys): update build script

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update build script

Signed-off-by: Xin Liu <[email protected]>

---------

Signed-off-by: Xin Liu <[email protected]>

* [Fix] Fix sys test

Signed-off-by: csh <[email protected]>

* Relax the version of `wat` dep  (#90)

* chore(rust-sdk): update `wat` dep

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sdk): update test code

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update test code

Signed-off-by: Xin Liu <[email protected]>

* version(rust-sys): bump to `0.17.4`

Signed-off-by: Xin Liu <[email protected]>

* version(rust-sdk): bump to `0.13.1`

Signed-off-by: Xin Liu <[email protected]>

* doc: update `CHANGELOG`

Signed-off-by: Xin Liu <[email protected]>

---------

Signed-off-by: Xin Liu <[email protected]>

* [CI] skip test_vmbuilder on fedora

Signed-off-by: csh <[email protected]>

* Fix static build to link agains zstd (#91)

Signed-off-by: Jorge Prendes <[email protected]>

* Update doc for releasing `v0.13.2` (#93)

* ci: update workflows

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update versioning table

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sdk): update versioning table

Signed-off-by: Xin Liu <[email protected]>

* ci(rust-static-lib): fix typo

Signed-off-by: Xin Liu <[email protected]>

* ci(rust-static-lib): update workflow

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update rustdoc

Signed-off-by: Xin Liu <[email protected]>

* version(rust-sdk): bump to `0.13.2`

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sdk): update versioning table

Signed-off-by: Xin Liu <[email protected]>

* ci(rust-static-lib): update workflow

Signed-off-by: Xin Liu <[email protected]>

---------

Signed-off-by: Xin Liu <[email protected]>

* [Version] Bump `wasmedge-sys` to `v0.17.5` (#94)

* version(rust-sys): bump to `0.17.5`

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update versioning table

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update dependency

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sdk): update versioning table

Signed-off-by: Xin Liu <[email protected]>

---------

Signed-off-by: Xin Liu <[email protected]>

* doc: update `CHANGELOG` (#95)

Signed-off-by: Xin Liu <[email protected]>

* [CI] Update `rust-static-lib` workflow (#96)

* ci(rust-static-lib): update workflow

Signed-off-by: Xin Liu <[email protected]>

* ci(rust-static-lib): update workflow

Signed-off-by: Xin Liu <[email protected]>

* ci(rust-static-lib): update workflow

Signed-off-by: Xin Liu <[email protected]>

* ci(rust-static-lib): update workflow

Signed-off-by: Xin Liu <[email protected]>

* ci(rust-static-lib): remove `libboost-all-dev` dep

Signed-off-by: Xin Liu <[email protected]>

---------

Signed-off-by: Xin Liu <[email protected]>

* [Fix] Temporary FuncRef extraction implementation.

Signed-off-by: csh <[email protected]>

* [CI] fix test wat

Signed-off-by: csh <[email protected]>

---------

Signed-off-by: csh <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Xin Liu <[email protected]>
Co-authored-by: Jorge Prendes <[email protected]>
Co-authored-by: Xin Liu <[email protected]>
@L-jasmine L-jasmine mentioned this pull request Dec 11, 2023
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