nodejs / uvwasi

WASI syscall API built atop libuv
MIT License
225 stars 49 forks source link

feat: start adding socket tests #214

Closed mhdawson closed 1 year ago

mhdawson commented 1 year ago

@ospencer this is the start of adding tests for sock_accept. I've commented out the part that does the actual sock_accept call since that is not implemented yet but it did already find one issue on the pre-open side of things.

Looking at the method a bit more since our discussion I think that possibly the on_new_connection method can simply call uv_cond_signal on a condition associated with the socket. Inside of sock_accept if the flags indicated that the call should be blocking then if an initial call to uv_accept returns UV_EAGAIN then it would call uv_cond_wait on that same condition associated with the sock.

I think that implementation for on_new_connection would mean that if previous blocking call to sock_accept had been made before the connection came in it would be woken up when a new accept is made, otherwise if no connection has been made when the sock_accept is called it will either return with UV_EAGAIN if the flags indicated it should be non-blocking or block waiting until the connection later came in. We'd also have to have some clean to be able to break out of the wait I think for shutdown.

mhdawson commented 1 year ago

@ospencer I experimented a bit based on the discussion with @cjihrig in https://github.com/nodejs/uvwasi/issues/211#issuecomment-1601615580 and added an initial implementation for uvwasi_sock_accept. Based on the uvwasi documentation I'm not sure we can depend on how calls to uv_accept are being used but it seems to pass initial test test when the connection is made before the call. Next thing will be to test when the connection comes in after the uv_accept call. The test needs an extra thread to do and I ran out of time before being able to add that.

mhdawson commented 1 year ago

:( CI seems to fail on mac os. I wonder if it might be because I've already closed the connection and I'm just getting lucky on linux. @ospencer any change you have a mac and can try/debug a bit there?

mhdawson commented 1 year ago

I pushed all of the tests so the client runs off thread and so that there is a delay after the connectionis made before the connection close, earlier tests passed on macos as well :). New tests were added to check the cases where the connection comes in after a blocking call and that's looking good a well. No tests actually test that we can send/receive on the socket we get from uvwasi_sock_accept but so far looking good.

mhdawson commented 1 year ago

@ospencer just so you are in the loop. Have also added initial implementations of uvwasi_sock_send and uvwasi_sock_receive. Right now using global vars etc. so not what we'll want in the end but shows how using the async libuv calls looks like.

cjihrig commented 1 year ago

@mhdawson is this ready for review?

mhdawson commented 1 year ago

@cjihrig it still needs some work before it might be ready to land. I've just found an issue that I'm current investigating while adding code in the test to close sockets, not sure if its a timing issue in the tests or a problem doing uv_accept. A few more things I think should happen before it is ready to land include:

Having said that a top level scan by yourself to see it's heading in the right direction/using reasonable approaches would be useful to avoid too much more work if the work so far is going in the wrong direction.

mhdawson commented 1 year ago

@cjihrig good news is that the accept issue was a problem in the uvwasi_sock_accept versus something in how uv_accept works :). Pushed a commit to fix.

One question, the libuv APIs don't seem to support closing the Read (versus write) part of the socket. Do you think we should throw a non-implemented error or simply add a flag to fd structure to mark that the socket has been closed for read and then return a non-writable error if reads are attempted on the socket later on.

mhdawson commented 1 year ago

@cjihrig accepted changes/pushed a commit to fix the 'easy' review comments. Now looking at the rest.

mhdawson commented 1 year ago

The tests look like they are likely still timing out on Windows and MacOS.

I had created a Windows machine and they run fine on my local machine so not sure what is going on since the action does not seem to show any output until they actually time out.

mhdawson commented 1 year ago

@cjihrig I think this is now ready for another review with the goal of getting it to lond.

The tests now run and pass on all of the CI platforms.

I believe I've addressed the comments except for using port 0. I'm not sure how we'd get the port that was used based on the available APIs.

I've also added validation so that we indicate when a requested option is not supported as well as the standard debugging which was missing before.