On client's read errors quick subsequent requests may wait for response indefinitely about tarpc HOT 9 CLOSED

BTW, I was able to refactor an existing test to consistently trigger this race: master...request_dispatch_race

from tarpc.

Thanks for raising this issue! I'll try to take a closer look this week, but also, feel free to send a PR for review.

from tarpc.

Could I see your code where the problem occurs? I'm curious how the RequestDispatch is being polled. When it hits an error, is it dropped?

from tarpc.

Can you see if this branch fixes the problem for you? https://github.com/tikue/tarpc/tree/request_dispatch_race

from tarpc.

Can you see if this branch fixes the problem for you? https://github.com/tikue/tarpc/tree/request_dispatch_race

Thanks a lot for implementing it @tikue! I tried it out and, unfortunately, this still results in the same problem. But your code LGTM in general. So I suspect that this may be due to the use of ready! here, which may be returned due to spurious failures and I think returns something else compared to the desired return Poll::Ready(Err(e));. Also note that the PR I created consistently doesn't have this problem.

from tarpc.

Hm, but the ready! macro only returns Poll::Pending if the underlying future being polled also returns Pending. The dispatch future should continue to be polled after that, right? If it's a spurious failure, than the mpsc receiver should have already arranged for a wakeup.

The problem with polling in a loop even when pending is returned is that it blocks in a nonblocking function. For example, a sender may have reserved a permit and then subsequently went to sleep for an hour. We don't want to block a Tokio thread for an hour waiting for the client to wake up, as there could be other async tasks that need to run in the meantime.

from tarpc.

Oh, true, you're right - the code should work just fine like that!

Also, I just noticed something strange in my tests. Although you changed the propagated error a little, my tests worked. But now when I try to reproduce my tests, I need to adjust the expected error message. Maybe I or cargo did something wrong, let me test it again.

from tarpc.

So the test has been running repeatedly for more than 20min now and no errors so far. Sorry for causing confusion earlier and thanks again for implementing the fix @tikue !

from tarpc.

That's great news, thanks for confirming! Yeah, I changed the client errors a little since more types of channel errors are propagated now. I might still revisit them a bit. (benefits of being perpetually pre-1.0...)

from tarpc.