fix: do not ignore result of ensure_recv_open
#687
Conversation
It may cause hanging, because result isn't checked
| stream.state.ensure_recv_open()?; | ||
| if !stream.state.ensure_recv_open()? { | ||
| return Poll::Ready(Ok(Response::new(()))); | ||
| } |
There was a problem hiding this comment.
So, if recv is NOT open, should it be returning a new empty response? Or is that an error in this context? It would be good to include a code comment here to remind us why, in either case.
There was a problem hiding this comment.
I was relying on similar logic here and here, it just forces to mark future as ready without error.
However, it's still not so clear what would be the more correct way, because Result<bool, Error> is confusing and probably should be refactored somehow. What I know for sure: bool shouldn't be ignored.
But for now I wanted to address the issue with simple & partially existed logic: if ensure_recv_open tells us it's not an error but stream is closed, it shouldn't be error. Otherwise we need to think how it can be refactored. Any ideas?
There was a problem hiding this comment.
Hm. Is there a way to build a test for this? That might help make it clearer what should actually be done.
Signed-off-by: Sven Pfennig <[email protected]>
It may cause hanging, because result isn't checked
Noticed within #600
For example, it's handled here and here with the similar logic