Fix uds listener hanging on accept

[mvucenovic]

UDS listener was hanging because the accept method would return Poll::Pending without registering the task to be awoken in the case when underlying unix listener returns a WouldBlock that gets converted to None. This is a hacky fix for this case.

Should fix #248

[skade]

This would be great chance to cover this with a test case to avoid regressions. @mvucenovic, would you be able to find a case that exhibits the (previously broken) behaviour?

[mvucenovic]

@skade Sure thing, I would like to write a bigger integration test that reproduces this issue, as I've mentioned in the #248. I won't be able to do this today, but will try to finish it before the weekend. I've submitted this PR early, because I consider this change a hacky one, so any suggestions how this should be done better are welcome.

[skade]

@mvucenovic Perfect 👍 thanks for letting me know!

[mvucenovic]

Hey @skade, I've added a test that reproduces the listener hanging forever. For me it fails on the master, but passes on this branch.

Interesting thing, my first version of test was not running in two different threads, but used join macro, to run server and client concurrently, and that version was green on the current master. It looked something like this:

     // rest of the code is mostly the same
     task::block_on(async {
         let listener = UnixListener::bind(&socket).await.expect("Socket bind");

         let server = ping_pong_server(listener, iter_cnt);
         let client = ping_pong_client(&socket, iter_cnt);

         async_std::io::timeout(
             Duration::from_secs(5),
             async_std::future::try_join!(server, client),
         ).await
     }).expect("not finished successfully");

I'll try to investigate why was that the case, just wanted to mention if somebody has any idea.

[jD91mZM2]

Hi, sorry for the notification, I just wanted to say that I needed to use unix sockets and switching to your fork solved it for me, just like that. You're awesome! :)

[ghost]

Just a few nits. :) Thanks!

[ghost]

@skade @yoshuawuyts It seems Clippy checks are failing in this pull request. Should we make Clippy tests not required to pass in CI?

[yoshuawuyts]

@stjepang the failure here was an upstream failure iirc that's now been fixed in actions-rs. Re-running the check should solve it. Clippy currently doesn't fail CI.

Additionally #270 should be landing soon now which means clippy errors should be resolved. This would allow us to then enable actions-rs/clippy-check#2 (comment) once it's implemented (if we so desire).

Anyway, going to restart the CI here which should fix the error!

[mvucenovic]

Thanks for the review @stjepang, I've updated the PR with your suggestions.

[ghost]

bors r+