[Fix] impl Drop for FiberFuture

[L-jasmine]

No description provided.

[juntao]

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

---

Overall Summary:

The pull request titled "[Fix] impl Drop for FiberFuture" contains two sets of changes. The first set adds the resume method to the FiberFuture struct and implements the Drop trait for the struct. The second set fixes a memory leak by properly handling the data pointer in the create_sync_func and create_async_func functions.

There are a few potential issues and errors that should be addressed. In the Drop implementation of FiberFuture, there is an assertion that assumes resumption with an error always completes the fiber. It would be good to add a comment explaining this situation and why the assertion can hold true in practice. Additionally, the reason for changing the error variant in function.rs should be documented.

The changes related to fixing the memory leak should be carefully reviewed and tested to ensure that they don't introduce new bugs. The reasoning behind changing the data parameter from Option<Box<T>> to *mut T should be confirmed and documented.

In summary, the changes appear to fix an issue with resuming fibers and improve the behavior of the FiberFuture struct during dropping. There is also a fix for a memory leak, but additional context about the problem and the solution would be beneficial. The potential issues and errors should be addressed to ensure the code is robust.

Details

Commit 033126ad7320b3d1093b3011fa1b873469b1edcd

Key changes:

• Added the resume method to the FiberFuture struct.
• Implemented the Drop trait for the FiberFuture struct.

Potential problems:

• In the Drop implementation of FiberFuture, there is an assertion debug_assert!(result.is_ok()) which assumes that the resumption with an error always completes the fiber. However, it mentions that it's technically possible for host code to catch the trap and re-resume. It would be good to add a comment explaining this situation and why the assertion can hold true in practice.
• In the file function.rs, the error variant HostFuncError::User(0x87) is changed to HostFuncError::Runtime(0x07) in both the wrap_async_wasi_fn and wrap_async_fn functions. It would be good to add a comment explaining the reason for this change.

Overall, the changes appear to fix an issue related to resuming fibers and improve the behavior of the FiberFuture struct during dropping. However, it would be good to address the potential problems mentioned above.

Commit 642e34165241051fce1dcd070315f8fc5eef8aae

Key changes in this pull request:

• Fixed a memory leak in several files by properly handling the data pointer in the create_sync_func and create_async_func functions.
• Replaced the Option<Box<T>> type for the data parameter with *mut T in the create_sync_func and create_async_func functions.
• Updated the function calls to pass the data parameter as &mut data as *mut _.

Potential Problems:

• The changes in the Function::create_sync_func and Function::create_async_func functions could introduce new bugs related to pointer handling. This should be carefully reviewed and tested.
• It's unclear why the data parameter was changed from Option<Box<T>> to *mut T. The reasoning behind this change should be confirmed and documented.
• Overall, the changes seem to be related to a memory leak issue, but additional context about the problem and the solution would be helpful for a better understanding.