feat: start adding socket tests

[mhdawson]

No description provided.

[mhdawson]

@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]

@ospencer I experimented a bit based on the discussion with @cjihrig in #211 (comment) 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]

:( 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]

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]

@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]

@mhdawson is this ready for review?

[mhdawson]

@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:

• look through other calls that accept fds and see if any are required for minimal functionality (for example closing sockets). Implement socket support for minimal set, and for the others possibly add code to return a reasonable error value.
• cleanup pass including fixing all ASAN issues.

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]

@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]

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

[mhdawson]

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]

@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.

[cjihrig]

Great work. Left some comments, but mostly just style things that can be applied from the GitHub UI. After merging this, it's probably worth cutting a new release.

[cjihrig]

Same comment about cutting back on malloc() and free() in the tests.

[mhdawson]

I think the difference is that this test uses multiple threads. There maybe be two threads using client_connection_thread at the same time.

For example in one part of the test we have

  // validate two accepts queue up properly
  start_client_connection_thread(&immediateThreadTime);
  start_client_connection_thread(&immediateThreadTime);
  start_client_connection_thread(&immediateThreadTime);

That applies to this case. For the other test we currenlty only have one instance but I think we should make it possible to have multiple launched to support future testing.

[mhdawson]

@cjihrig since this is PR against a side branch I'm going to land the PR and then create a PR against the main branch. We can continue the discussion there if its ok to leave the remaining mallocs there due to the multi-threaded nature of the tests.