[WASI]: Unify the write and the read function. #2643

MediosZ · 2023-07-03T12:55:10Z

This PR trys to unify the read and write function to address the issue when using epoll.
The current implementation causes epoll being triggered endlessly.

juntao · 2023-07-03T12:55:12Z

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

Overall, the GitHub Pull Request is aimed at unifying the read and write functions in the WASI module, extending compatibility beyond Linux to MacOS and Windows systems. This is done mainly through introducing a 'process' function that takes care of read and write operations on file descriptors, expanding the 'inode-linux.cpp' code to handle this function, and refactoring 'pollOneoff' in the 'WasiFunc.cpp' file to alter how it handles read and write events.

The most eminent issues in the pull request are tied to error handling, code duplication, performance, and testing. Firstly, there are a variety of points where exceptions might be thrown leading to potential bugs and security risks, which the current error handling is not equipped to deal with robustly. The use of epoll_ctl is particularly troubling, as errors are assumed to be rare and aren't handled in a robust manner. This, coupled with the lack of unit tests to confirm the reliability and correctness of new features, results in serious concerns over future stability.

Secondly, there is an issue with code duplication, as the 'process' function in 'inode-macos.cpp' and 'inode-win.cpp' contain identical patterns which could instead be refactored into a helper function.

Lastly, the current patch could lead to performance issues. There are potential inefficiencies linked to the use of an unordered_map dependent on the File Descriptor in the loop within the pollOneoff function. Also, implementation of conditional flags checks for ReadFlag and WriteFlag can increase system complexity and affect performance negatively. This highlights the need for performance assessment and optimization.

Therefore, while the pull request proposes useful functional unifications, the absence of robust error handling, adequate testing, efficient coding and performance considerations may invite future bugs and bottlenecks.

Details

Commit fb00f8bf4724824d613ed2e7d91befafb93afa46

The proposed GitHub patch attempts to unify the write and read functions in the WASI module of the application. First, it introduces the 'process' using function to manage the read/write processes throughout the 'include/host/wasi/environ.h', 'include/host/wasi/inode.h', and 'include/host/wasi/vinode.h' files.

Major changes include:

Added a 'process' function in 'EVPoller', 'Poller', and 'VPoller' classes which manages both the read and write operations on file descriptors.
Extended the 'inode-linux.cpp' file to handle the process function for different conditional flags checking for ReadFlag and WriteFlag.
Refactoring the 'pollOneoff' in 'WasiFunc.cpp' file to modify how it handles the case of Read and Write events.

However, there could be potential problems with this patch. One major concern is the error handling and exception safety. The newly-added code contains several potential points where an exception may be thrown (std::bad_alloc), and overall error handling might not be robust. In several places (e.g., epoll_ctl calls) errors are assumed to be "unlikely" but are not handled in a way that is consistent or robust, potentially leading to future bugs or security issues.

Also, the patch doesn't provide test cases for the new functionality. It's risky to integrate new changes without having proper tests constructed to ensure that everything works as expected.

Lastly, some code may cause performance issues due to the loop in the pollOneoff function involving an unordered_map dependent on the File Descriptor. Depending on the incoming traffic, this can cause significant performance overhead.

In summary, while the patch attempts to unify the write and read functions, without robust error handling, exception safety, performance assessment, and test cases, it may introduce future bugs and potential performance bottlenecks.

Commit 58e643ac6009adb5eef742ad6788f00f1f001852

Summary:

The pull request aims to unify the "read" and "write" functions in the WebAssembly System Interface (WASI) for different operating systems like MacOS and Windows. The developer removed some of the Linux-specific preprocessor directives in "inode.h" and "inode-linux.cpp" files and added generic function process() . The functionality for checking if both the read and write flags are set, previously available only for Linux, is now implemented for MacOS and Windows as well.

Potential Issues:

No check for success or failure: When the "read" or "write" functions are called, there does not appear to be any code to check whether these operations succeeded or failed. This could potentially result in silent failures.
Lack of unit tests: The developer has not provided any unit tests to assert the correctness of the new features implemented, which is necessary for every PR to ensure the quality and reliability of the code.
Duplication of code: The "process" function in both "inode-macos.cpp" and "inode-win.cpp" files have code duplication. The same pattern of checking the flags and calling "read" and "write" occurs. This could have been factorized into a helper function to avoid repeating code.
Impact on performance: The conditions to check if both flags are marked or if only one of them is, followed by calling "read" and "write" functions, can increase the complexity and potentially impact system performance. Verification and optimization of this system are needed.

codecov · 2023-07-03T13:07:28Z

Codecov Report

Merging #2643 (58e643a) into master (c461f98) will decrease coverage by 0.37%.
The diff coverage is 47.59%.

@@            Coverage Diff             @@
##           master    #2643      +/-   ##
==========================================
- Coverage   81.20%   80.84%   -0.37%     
==========================================
  Files         148      148              
  Lines       21473    21614     +141     
  Branches     4313     4369      +56     
==========================================
+ Hits        17437    17473      +36     
- Misses       2877     2975      +98     
- Partials     1159     1166       +7

Impacted Files	Coverage Δ
include/host/wasi/inode.h	`69.23% <ø> (ø)`
include/host/wasi/vinode.h	`51.82% <43.75%> (-0.81%)`	⬇️
include/host/wasi/environ.h	`66.58% <44.44%> (-0.47%)`	⬇️
lib/host/wasi/inode-linux.cpp	`49.37% <44.53%> (-3.98%)`	⬇️
lib/host/wasi/wasifunc.cpp	`48.55% <68.18%> (+0.24%)`	⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Tricster <mediosrity@gmail.com>

hydai · 2023-07-05T11:28:19Z

Hi @MediosZ
I got a little bit confused. Since there are only two functions, read and write in WASI spec, I don't think providing a new function and combining both functions is a good idea. This issue is triggered by modifying the READ or WRITE every time. However, we can change the behavior to the following.

Case 1. If it's new, just apply READ or WRITE.
Case 2. It already has READ or WRITE, then we modify it.

MediosZ · 2023-07-06T07:43:23Z

I feels like it's hard to cover all the cases with these two functions.

Let's say we have registered READ and WRITE in the first call.
And in the second call, we receive only the READ subscription only, which means only the read function will be invoked and we can not determine if the write function will be invoked.
In this case, we should not modify the epoll since the READ doesn't change. As a result, the WRITE will not be deregistered as the write function will not be invoked.

That's why I was think of unify these two functions as that's how epoll works.

hydai · 2023-07-18T08:04:35Z

include/host/wasi/vinode.h

@@ -758,6 +759,35 @@ class VPoller : protected Poller {
      Poller::write(Fd->Node, Trigger, UserData);
    }
  }
+
+  void process(std::shared_ptr<VINode> Fd [[maybe_unused]],


Why do we need so many [[maybe_unused]] attributes here?

hydai · 2023-07-18T08:15:19Z

include/host/wasi/vinode.h

+               bool ReadFlag [[maybe_unused]], bool WriteFlag [[maybe_unused]],
+               __wasi_userdata_t ReadUserData [[maybe_unused]],
+               __wasi_userdata_t WriteUserData [[maybe_unused]]) noexcept {
+    auto TempReadFlag = ReadFlag;


Temp is so confusing. Please use a precise name.

hydai · 2023-07-18T08:18:37Z

lib/host/wasi/inode-linux.cpp

+
+      epoll_event EPollEvent;
+      EPollEvent.events = EPOLLIN;
+      EPollEvent.events |= EPOLLOUT;


Why not:

EPollEvent.events = EPOLLIN | EPOLLOUT;

hydai · 2023-07-18T08:22:17Z

lib/host/wasi/inode-linux.cpp

+          return;
+        }
+      } else {
+        //


Redundant comments.

hydai · 2023-07-18T08:23:40Z

lib/host/wasi/inode-linux.cpp

+      WriteEvent.error = __WASI_ERRNO_NOMEM;
+      return;
+    }
+


Redundant empty line.

hydai · 2023-07-18T08:29:38Z

lib/host/wasi/inode-linux.cpp

+        }
+      } else {
+        if ((ReadFlag && OldIter->second.ReadEvent == nullptr) ||
+            (WriteFlag && OldIter->second.WriteEvent == nullptr)) {


Please simplify the statement in the current condition (ReadFlag = false && WriteFlag = true).

hydai · 2023-07-18T08:31:44Z

lib/host/wasi/wasifunc.cpp

+        auto Iter = SubInfoMap.find(WasiFd);
+        const bool NotFound = Iter == SubInfoMap.end();
+        if (NotFound) {
+          SubInfo Info = {.ReadFlag = true,


Don't use this pattern. It's not in the C++17 standard. It's c99 standard, and will be shipped in C++20 (ref: https://www.cppstories.com/2021/designated-init-cpp20/)

hydai · 2023-07-18T08:34:13Z

lib/host/wasi/wasifunc.cpp

+            Iter->second.ReadData = WasiUserData;
+          }
+        }
+        // Poller.read(WasiFd, Trigger, WasiUserData);


Why comment this out?

hydai · 2023-07-18T08:34:22Z

lib/host/wasi/wasifunc.cpp

+        auto Iter = SubInfoMap.find(WasiFd);
+        const bool NotFound = Iter == SubInfoMap.end();
+        if (NotFound) {
+          SubInfo Info = {.ReadFlag = false,


hydai · 2023-07-18T08:34:31Z

lib/host/wasi/wasifunc.cpp

+            Iter->second.WriteData = WasiUserData;
+          }
+        }
+        // Poller.write(WasiFd, Trigger, WasiUserData);


github-actions bot added the c-WASI label Jul 3, 2023

MediosZ added 2 commits July 4, 2023 09:20

[WASI]: Unify the write and the read function.

fb00f8b

Signed-off-by: Tricster <mediosrity@gmail.com>

[WASI] Add workaround for macos and windows.

58e643a

Signed-off-by: Tricster <mediosrity@gmail.com>

MediosZ force-pushed the epoll branch from 61ed7fa to 58e643a Compare July 4, 2023 01:34

hydai requested changes Jul 18, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WASI]: Unify the write and the read function. #2643

[WASI]: Unify the write and the read function. #2643

MediosZ commented Jul 3, 2023

juntao commented Jul 3, 2023 •

edited

codecov bot commented Jul 3, 2023 •

edited

hydai commented Jul 5, 2023

MediosZ commented Jul 6, 2023

hydai Jul 18, 2023

hydai Jul 18, 2023

hydai Jul 18, 2023

hydai Jul 18, 2023

hydai Jul 18, 2023

hydai Jul 18, 2023

hydai Jul 18, 2023

hydai Jul 18, 2023

hydai Jul 18, 2023

hydai Jul 18, 2023

[WASI]: Unify the write and the read function. #2643

Are you sure you want to change the base?

[WASI]: Unify the write and the read function. #2643

Conversation

MediosZ commented Jul 3, 2023

juntao commented Jul 3, 2023 • edited

Details

codecov bot commented Jul 3, 2023 • edited

Codecov Report

hydai commented Jul 5, 2023

MediosZ commented Jul 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juntao commented Jul 3, 2023 •

edited

codecov bot commented Jul 3, 2023 •

edited