Simplify Future for RequestDispatch - #554
Conversation
2b4f63b to
728d8af
Compare
|
Bump. |
|
Hello? |
|
Sorry for the delay, I'll take a look soon. |
|
Bump. |
| .clone() | ||
| .downcast() | ||
| .expect("Invariant: ChannelError must store a C::Error"); | ||
| ready!(self.shut_down_with_terminal_error(cx, e.clone().upcast_error())); |
There was a problem hiding this comment.
I'm not sure this change works, because shut_down_with_terminal_error can return Pending and then be called again. The request dispatcher closes the pending requests channel and then waits for the other end of the channel to be closed, so Pending can be returned if the other end isn't immediately closed.
Sorry that there aren't good tests for this that would have caught this problem earlier. Would you like to try adding a test?
There was a problem hiding this comment.
Instead, there can be a field like
Arc<OnceCell<SomeError>>shared between dispatch and channel, so that if writing to channel or reading back from oneshot channel fails, we could attach that error as error source.
That sounds good too, but won't there be a regression if this PR is merged without the new field?
There was a problem hiding this comment.
Good catch, thanks! This PR is now much simpler, only removes Any and uses generics. But most of complexity I wanted to remove, stays.
I used AI to add tests that reproduce the problem. Tests are not very readable, not sure how much value in them.
|
Apologies again for the long delay. Life got in the way. |
cce6864 to
8b8c31c
Compare
8b8c31c to
79427a3
Compare
2 participants
Stop type-erasing
RequestDispatch::terminal_error.The field is already on a type generic over the transport, so it can be
Option<ChannelError<C::Error>>. That removesChannelError::{upcast_any, downcast}and thedyn Anyround-trip inpoll.The context is this:
terminal_erroronly works for inflight requests, but thisis not enough to provide good diagnostics if we are sending request to a
dispatch which has already failed.
Instead, there can be a field like
Arc<OnceCell<SomeError>>shared betweendispatch and channel, so that if writing to channel or reading back from oneshot
channel fails, we could attach that error as error source.
So I start by cleaning up existing code.