From d106c615bc0c289ba6d1ce6871786266d109c31c Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Fri, 27 Jul 2018 14:17:19 +0100 Subject: [PATCH] Don't check StreamingCallSink is alive at close There is nothing in place which guarantees that sending the last message on the Sink, and closing the Sink, happen atomically. There is currently a race condition where if the server responds incredibly quickly, its response may be received between the last streaming message being sent, and close being called. It isn't obvious to me what value this check_alive call is bringing; if there is one, we should try to guarantee its success by either: 1. Implementing a custom send_all implementation such that the last send and close are called atomically, getting rid of the window where the race can be lost. 2. Establishing a happens-before relationship between StreamingCallSink.close and whatever code receives the server response (I think Call.start_recv_message?). I think this would either need a pretty coarse-grained lock, or a pretty complex restructuring of code. But as the check doesn't appear to provide any useful guarantees (and indeed mostly appears to make losing this race condition problematic), I figured I'd try just removing it... --- src/call/client.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/call/client.rs b/src/call/client.rs index a16a9d12a..5b580cb11 100644 --- a/src/call/client.rs +++ b/src/call/client.rs @@ -351,7 +351,6 @@ impl Sink for StreamingCallSink { if let Async::NotReady = self.close_f.as_mut().unwrap().poll()? { // if call is finished, can return early here. - call.check_alive()?; return Ok(Async::NotReady); } Ok(Async::Ready(()))