proto: Make DataWriter::close actually do something.
Previously we had a bug where <DataWriter as AsyncWrite>::close
(or shutdown in tokio-land) would not actually have any effect.
It would drop the StreamTarget held by the DataWriter, but
since the DataReader also held a StreamTarget, the
MPSC channel would not get closed, and the circuit reactor would
not realize that the stream wanted to shut down.
Now we use mpsc::Sender::close_channel to make our closes
effectual.
Closes #1368 (closed).
Merge request reports
Activity
requested review from @nickm
assigned to @nickm
mentioned in issue #1368 (closed)
added 1 commit
- 4bd59c01 - proto: Improve documentation about DataStream lifetimes and closing
removed review request for @nickm
requested review from @jnewsome
564 564 self.state = Some(DataWriterState::Closed); 565 565 Poll::Ready(Err(e.into())) 566 566 } 567 Poll::Ready((imp, Ok(()))) => { 567 Poll::Ready((mut imp, Ok(()))) => { 568 568 if should_close { 569 // Tell the StreamTarget to close, so that the reactor 570 // realizes that we are done sending. (The Reader has its own 571 // StreamTarget, so just dropping ours would not be sufficient for -
It's a little surprising that the reader has its own
StreamTarget. Digging a little, I see that it's so that it can signal that the application is reading (by sendingsendmes), and so that it can signal protocol errors.The former might eventually go away once we no longer support pre-prop324 flow control, but that's a long ways off. I suppose it'd still need it for signaling errors though, barring some further refactor to move that up a level or something. In any case, I agree that doing an explicit close seems more robust than just dropping the object.
Maybe tweak the comment here along the lines of "it's difficult to be certain that no other clones of the sender, and in particular
StreamReadercurrently has its own copy"? It'd also be helpful to comment thetargetfield inStreamReaderto explain a bit why it has it. changed this line in version 3 of the diff
-
Maybe the repro in #1368 (closed) could be checked in as a standalone test?
-
Oh, I just realized that talks to the real network.
It could probably be adapted to something that runs under shadow against the test network. It might be a little work to write the first such test, but it seems like a useful strategy in general. If you want you can file an issue to set up a framework for such tests, starting with this repro.
Of course, if you have an idea for a simpler regression test in the meantime that'd be good too :)
You could accomplish the same thing with a temporary onion service as the endpoint. I have a functional test in my
tor-interfacecrate which discovered this behaviour:
marked this merge request as draft from nickm/arti@1f036c5e
requested review from @jnewsome and removed approval
2037 let (r, mut w) = stream.split(); 2038 if by_drop { 2039 // Drop the writer and the reader, which should close the stream. 2040 drop(r); 2041 drop(w); 2042 (None, circ) // make sure to keep the circuit alive 2043 } else { 2044 // Call close on the writer, while keeping the reader alive. 2045 w.close().await.unwrap(); 2046 (Some(r), circ) 2047 } 2048 }; 2049 let handler_fut = async { 2050 // Read the BEGIN message. 2051 let (_, msg) = rx.next().await.unwrap().into_circid_and_msg(); 2052 let rmsg = match msg { 2054 AnyRelayMsgOuter::decode_singleton(RelayCellFormat::V0, r.into_relay_body()) 2055 .unwrap() 2056 } 2057 _ => panic!(), 2058 }; 2059 let (streamid, rmsg) = rmsg.into_streamid_and_msg(); 2060 assert_eq!(rmsg.cmd(), RelayCmd::BEGIN); 2061 2062 // Reply with a CONNECTED. 2063 let connected = 2064 relaymsg::Connected::new_with_addr("10.0.0.1".parse().unwrap(), 1234).into(); 2065 sink.send(rmsg_to_ccmsg(streamid, connected)).await.unwrap(); 2066 2067 // Expect an END. 2068 let (_, msg) = rx.next().await.unwrap().into_circid_and_msg(); 2069 let rmsg = match msg { 
I'd rather solve this in one place with #1426, given how many times this pattern appears.
@nickm invited me to take a look at this.
I have reviewed the new API docs and commentary. This all looks good to me. I think the behaviour described is the desired behaviour.
I haven't reviewed the implementation in detail, nor double checked that the new test case expects the same behaviour as is documented. I'm trusting @jnewsome to do that as part of his review :-).
mentioned in commit ee96fa6f