refraction-networking / water Goto Github PK

It is confirmed that after a connections gets closed, the underlying resources are not properly freed and therefore the WebAssembly runtime/module/instance continue to exist and therefore holding all the memory allocated.

Original post

By running the listener example, I keep seeing growing Alloc and Sys. Waiting between handling incoming connections will not help reducing the memory allocation.

One of the external dependency of water, tetratelabs/wazero has been replaced with our fork, refraction-networking/wazero. However, Go itself will not inherit the replace directive in go.mod and therefore any dependents of this project must manually add the replace directive to their go.mod as well, making it challenging to maintain.

We used wazero.ModuleConfig in WazeroModuleConfigFactory:

https://github.com/gaukas/water/blob/1acc17fba677cb505b8cd366dcb9ce49c226535e/wazero_config.go#L12-L15

However, by default all these system critical components are unset/set-to-noop/set-to-fake instead of default to using system-default.

At least, we must explicitly invoke:

• WithSysWalltime()
• WithSysNanotime()
• WithSysNanosleep()
• WithRandSource(io.Reader)

In #11 benchmarks for Dialer/Listener (v0) have been rewritten, with a lot more test suite implemented to provide a more thorough and detailed test.

However the throughput measured become somehow even better than raw TCP connections created with net.Dial() and net.Listener.Accept().

goos: linux
goarch: amd64
pkg: github.com/gaukas/water/transport/v0
cpu: 12th Gen Intel(R) Core(TM) i5-1240P
BenchmarkDialerOutbound
BenchmarkDialerOutbound-16        931250              1207 ns/op         848.10 MB/s
BenchmarkListenerInbound
BenchmarkListenerInbound-16       806343              1510 ns/op         678.06 MB/s
BenchmarkTCPReference
BenchmarkTCPReference-16          533594              1880 ns/op         544.66 MB/s 

This does not look right. Even if the result is true, we require an explanation for why should WebAssembly-based transport be faster than native TCP sockets.

Additionally, we will require more test suites, plus more sample WATMs in testdata/v0 to demonstrate:

• Bytes are indeed processed by WATM (currently, plain.wasm does not process bytes which leads to possible evasion risks)
• Latency is not drastically increased by WATM (currently not measured)

Current implementation heavily relies on CGO and Rust FFI which are assumed to be problematic in terms of portability and performance.

To eliminate such issues, we plan to switch to a pure Go implementation using a pure Go WebAssembly runtime with WASI support (current candidate: wazero).

The Pull Request tetratelabs/wazero#1903 or its equivalent will patch wazero's async socket read and therefore fixes the failing fd_read() blocking water from working on Windows. After wazero merges it (or later, when a new wazero version is released after), water should undo skipping tests on Windows.

While water-rs has covered the examples of Rust WATM and supporting libraries, we should do the same for Go (TinyGo only, until Go officially implements //go:export).

Currently both transport/v0 and transport/v1 hardcodes TCPConn for local WASI usage (caller <==> WATM) and UnixConn is unused. It would be potentially interesting to allow the use of UnixConn by caller's choice.

We need more tests to demonstrate cross-platform/cross-architecture feature.

• linux-amd64
• linux-aarch64
• linux-riscv64 (which?)
• macOS-amd64
• macOS-aarch64
• windows-amd64
• windows-aarch64

Observation

When (*Config).TransportModuleConfig is set, call to (*core).Instantiate() will fail with error message: water: memFS.WriteFile returned error: success.

Expected Behavior

The call to (*core).Instantiate() should either succeed or fail with an intuitive error. "success" is not considered an intuitive error.

Problem

The above logic is incorrect. To ignore nil error and system errno 0, the correct condition for the if statement will be:

if !errors.Is(err, nil) && !errors.Is(err, sys.Errno(0))

WASI Preview 2, aka WASI 0.2.0 is now stable. It would greatly benefit this project by introducing official wasi-sockets support.

We should evaluate WASI Preview 2 and its support from each WebAssembly runtime to determine when would it be viable for WATER to build a Transport Module version based on wasip2.

It is feasible to include a standalone testing package for watm to be run for every newly created WATM. This also helps with the CI/CD pipeline of the watm repository by filtering out faulty artifacts before releasing.

This error was introduced at some point:

--- FAIL: BenchmarkConnV0/PlainV0-Listener
    conn_test.go:171: avg bandwidth: 8.791524 MB/s (N=1)
    conn_test.go:174: water: writing to cancel pipe failed: write unix /tmp/2260acd271452e8d1f3e65e626f2776f->@: write: broken pipe

Trying to identify the cause.

Note: This is believed to be a wasi-related problem. However, until it is resolved, user should pay extra care when dealing with it.

Problem

For some reason when a TCP Connection is closed by the remote by a FIN packet, even tho the Go will be able to detect that and return EOF on the next Read(), the async read (by tokio) in WebAssembly may not be able to correctly handle that. In rare cases, the non-blocking async read becomes blocking and does not return EOF immediately, and meanwhile preventing other async operations from making progress, thus halting the whole system.

Additional Context

This bug is not a runtime-side bug, but it is non-trivial to fix or work around inside the WebAssembly environment. Therefore from the runtime-side we work around it by using a unix socket to relay between the network socket and the WebAssembly environment. When the runtime detects that the network socket is closed, it will close the unix socket, which will immediately unblock the async read in WebAssembly.

Solutions

There's not yet a clear fix to this problem.

We introduced connhaltbug build tag in #7, which patch this issue with the workaround mentioned above. A formal patch will require further investigation.

As our paper at FOCI is finalized and this project becomes officially enlisted by refraction, we are transferring this repository to be owned by refraction for future references and further maintenance.

Need to follow the guideline here: https://pkg.go.dev/license-policy

Perhaps we should revert back to Apache-2.0 only. go.dev doesn't like our dual licensing and cannot display content correctly.

Requesting insights on performance between UnixConn and TCPConn under different scenarios.

Is the prerequisite of: #68

It would be feasible to provide a method allowing (partially) constructing Config from a file/stream of bytes/human-readable text string input, or other types of serializable data.

We will need a new type package, ConfigBuilder internal/configbuilder, as which provide an interface(s) supporting building Config from serializable data.

Candidate Formats:

• Protobuf
• JSON
• YAML

We want to introduce a breaking change before hitting stable (v1) to use the negative version of syscall Errno to replace the custom errcode defined below:

https://github.com/gaukas/water/blob/d382a44fed241087a6a3f8d4e87dc89d5bcf8443/internal/wasm/errors.go#L9-L20

Using the negative Errno saves potentially non-trivial translation between syscall and API, and is more intuitive to those who have background in dealing with syscall.

Cause

In e8cc8e6 we patched the unsafe defer as suggested by deepsource and therefore we are returning with the error returned by .Close() in a few functions.

Example

(net.Listener).Close() returns a non-crashing error without impacting the program safeness/liveness.

https://github.com/gaukas/water/actions/runs/6473922694/attempts/1

Suggested Fix

• No fix will be applied on water/socket library (otherwise it will be essentially unsafe again).
• In water when calling socket.UnixConnPair(), caller should proceed if returned error pair is not nil even when the error is non-nil.

We need documentations for the standardized API for WATM v0 to help understanding the system architecture of this over-complicated project.

In #37 I realized the lack of documentation on RegisterDialer, RegisterListener, and RegisterRelay is confusing given they are exported functions that were not supposed to be called by a general user(caller).

Considering moving them into an internal package. It was left public and in the root directory to allow third party transport module version support. So add better documentation and optionally move it into another package WITHOUT making it internal is better.

Also, there's not enough document nor example emphasizing the corresponding version of transport module support (e.g., _ "github.com/gaukas/water/transport/v0") is required. Documentation needed for that. Hopefully I did a good job in addressing this part in #39.

Since WATM v1 driver is already implemented and merged in #65, we should update README.md and any other related documentation to reflect the change we have made to introduce the new driver and any new features come along with the v1 driver.

The code for ExampleXxx is highly redundant and unclear, it is better if we could further simplify it.

Plus, consider breaking ExampleDialer by moving definitions for internal variables out of dialer_test.go. Currently go.dev generated a runable example for ExampleDialer but in fact it won't run since go.dev does not have a copy of testdata:

Output:

go: downloading github.com/gaukas/water v0.4.0
go: downloading github.com/tetratelabs/wazero v1.6.0
go: downloading google.golang.org/protobuf v1.32.0
prog.go:15:13: pattern transport/v0/testdata/plain.wasm: no matching files found

We can (and should) use wazero.CompilationCache to save time by avoiding repetitively building the same .wasm. This may bring crucial performance improvement for non-I/O phase (i.e., bootstrapping speed).

It is not the best practice to include one module as a submodule of another. For now, go cannot obtain the latest version correctly.

So instead it seems the most proper way is separating them into two repositories, instead of putting them all under water as submodule or watm as a package under water module (which would bring mess to water's go.mod).

Hi there, congratulations on getting this project to this state, it looks quite promising!

As part of the development of Outline and Outline SDK, I played with interfaces a lot. One thing that became clear to me is that the Go dial API isn't that great.

Most critically, Dial returns a net.Conn, which doesn't support TCP semantics. To support TCP, you need to be able to close the write end (which sends a FIN). While TCPConn has CloseRead() and CloseWrite(), net.Conn does not. That's a big omission, and the reason why we use StreamConn instead in the Outline SDK. You must be able to indicate you are done sending without tearing down the entire connection.

Other less important issues, perhaps personal preference:

• DialContext is unnecessarily verbose, and having two Dials is redundant and confusing. Just make Dial take a context.Context.
• The network parameter in the Dial is unnecessary, and a violation of the encapsulation. It leaks the implementation of the Dialer. Want to dial using TCP? Use a TCP dialer. Want to use IPv6? Use a dialer that uses IPv6. In the Outline SDK, we explicitly have different calls for stream and datagram (packet) dial and connections to give the developer stronger typing, since the connections are semantically different.