Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2050483002:
[Cronet] Coalesce small buffers into single QUIC packet in GRPC on iOS. (Closed)
Description[Cronet] Coalesce small buffers into single QUIC packet in GRPC on iOS.
Added 'disable_auto_flush' and 'delay_request_headers_until_flush' to cronet_c_for_grpc API.
BUG=601972
Committed: https://crrev.com/62d0ba2a00cd3eeca6afa9edc572829a711410b8
Cr-Commit-Position: refs/heads/master@{#400009}
Patch Set 1 #Patch Set 2 : Added tests and made them pass. #Patch Set 3 : Add more tests. #Patch Set 4 : A little test cleanup. #
Total comments: 27
Patch Set 5 : Sync #Patch Set 6 : Address Helen's comments, play with gn. #Patch Set 7 : Change condition. #Patch Set 8 : Merge SendFlushingWriteData and FlushDataOnNetworkThread. #
Total comments: 30
Patch Set 9 : Address comments. #Patch Set 10 : Remove flushing_write_data_. #
Total comments: 21
Patch Set 11 : Reintroduce flushing_write_data. #
Total comments: 7
Patch Set 12 : Sync #Patch Set 13 : Address Helen's comments. #
Total comments: 2
Messages
Total messages: 33 (5 generated)
Description was changed from ========== Add 'disable_auto_flush' and 'delay_request_headers_until_flush' to cronet_c_for_grpc API. BUG= ========== to ========== [Cronet] Coalesce small buffers into single QUIC packet in GRPC on iOS. Added 'disable_auto_flush' and 'delay_request_headers_until_flush' to cronet_c_for_grpc API. BUG=601972 ==========
mef@chromium.org changed reviewers: + kapishnikov@chromium.org, xunjieli@chromium.org
PTAL.
Sorry for the delay. I was trying to fix something else. https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:136: if (!request_headers_sent) { Should we not check |write_end_stream_|? Currently if it is a GET request and |delay_headers_until_flush_| is set, we delay the headers until flush() in Java. I wonder whether we should do the same. https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:185: DCHECK(write_state_ == WRITING); DCHECK_EQ? https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:194: SendFlushingWriteData(); Maybe invoke FlushOnNetworkThread() directly? This is match the Java implementation more closely. Having one fewer method is easier to manage. https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:286: if (!pending_write_data_) { Also need to check |flushing_write_data_| ? https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:313: if (write_state_ != WAITING_FOR_FLUSH) { Flush() can be called anytime after OnStreamReady() (even after stream is destroyed), so we need to handle that. In FlushOnNetworkThread, maybe check that write_state_ == WAITING_FOR_FLUSH, if not, just accumulate the pending write data in the flushing write data? Suggest combining this method with FlushOnNetworkThread() method. It might be slightly easier to reason. https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_bidirectional_stream.h (right): https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.h:58: // Disable automatic flushing of each buffer passed to WriteData(). nit: s/Disable/Disables ? https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.h:63: // Delay sending request headers until first call to Flush(). nit: s/Delay/Delays https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.h:88: // Send buffers passed to WriteData(). nit: s/Send/Sends or maybe Flushes buffer passed through WriteData() to the native stack. https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_c_for_grpc.h (right): https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_c_for_grpc.h:196: /* Write request data from |buffer| of |buffer_length| length. If auto flush is nit: Writes? https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_c_for_grpc.h:211: * Flush pending writes. This method should not be called before invocation of nit: Flushes?
Thanks! https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:136: if (!request_headers_sent) { On 2016/06/10 16:30:49, xunjieli wrote: > Should we not check |write_end_stream_|? Currently if it is a GET request and > |delay_headers_until_flush_| is set, we delay the headers until flush() in Java. > I wonder whether we should do the same. But that would never come if auto flush is enabled, right? https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:185: DCHECK(write_state_ == WRITING); On 2016/06/10 16:30:49, xunjieli wrote: > DCHECK_EQ? Done. https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:194: SendFlushingWriteData(); On 2016/06/10 16:30:50, xunjieli wrote: > Maybe invoke FlushOnNetworkThread() directly? This is match the Java > implementation more closely. Having one fewer method is easier to manage. I think they are different. FlushOnNetworkThread is posted from app call to Flush() at any time. SendFlushingWriteData() is called when we are ready to send data that was accumulated for flushing. https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:286: if (!pending_write_data_) { On 2016/06/10 16:30:50, xunjieli wrote: > Also need to check |flushing_write_data_| ? Hrm, I think if |flushing_write_data_| is not null, then it means that FlushOnNetworkThread was called before, and headers will be sent with the data. https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:313: if (write_state_ != WAITING_FOR_FLUSH) { On 2016/06/10 16:30:50, xunjieli wrote: > Flush() can be called anytime after OnStreamReady() (even after stream is > destroyed), so we need to handle that. > > In FlushOnNetworkThread, maybe check that write_state_ == WAITING_FOR_FLUSH, if > not, just accumulate the pending write data in the flushing write data? > > Suggest combining this method with FlushOnNetworkThread() method. It might be > slightly easier to reason. SendFlushingWriteData is similar to nativeWritevData, it never gets called directly by the app, only from FlushOnNetworkThread or onDataSent when previous set of buffers has flushed. https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_bidirectional_stream.h (right): https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.h:58: // Disable automatic flushing of each buffer passed to WriteData(). On 2016/06/10 16:30:50, xunjieli wrote: > nit: s/Disable/Disables ? Done. https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.h:63: // Delay sending request headers until first call to Flush(). On 2016/06/10 16:30:50, xunjieli wrote: > nit: s/Delay/Delays Done. https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.h:88: // Send buffers passed to WriteData(). On 2016/06/10 16:30:50, xunjieli wrote: > nit: s/Send/Sends > or maybe > Flushes buffer passed through WriteData() to the native stack. Done. https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_c_for_grpc.h (right): https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_c_for_grpc.h:196: /* Write request data from |buffer| of |buffer_length| length. If auto flush is On 2016/06/10 16:30:50, xunjieli wrote: > nit: Writes? Done. https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_c_for_grpc.h:211: * Flush pending writes. This method should not be called before invocation of On 2016/06/10 16:30:50, xunjieli wrote: > nit: Flushes? Done.
https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:136: if (!request_headers_sent) { On 2016/06/10 18:36:36, mef wrote: > On 2016/06/10 16:30:49, xunjieli wrote: > > Should we not check |write_end_stream_|? Currently if it is a GET request and > > |delay_headers_until_flush_| is set, we delay the headers until flush() in > Java. > > I wonder whether we should do the same. > > But that would never come if auto flush is enabled, right? You are right. Hopefully we will get rid of auto flush soon, so we don't run into this case. https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:306: if (!sending_write_data_) Can this check be just "if (write_state_ != WRITING)"? https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:313: if (write_state_ != WAITING_FOR_FLUSH) { On 2016/06/10 18:36:36, mef wrote: > On 2016/06/10 16:30:50, xunjieli wrote: > > Flush() can be called anytime after OnStreamReady() (even after stream is > > destroyed), so we need to handle that. > > > > In FlushOnNetworkThread, maybe check that write_state_ == WAITING_FOR_FLUSH, > if > > not, just accumulate the pending write data in the flushing write data? > > > > Suggest combining this method with FlushOnNetworkThread() method. It might be > > slightly easier to reason. > > SendFlushingWriteData is similar to nativeWritevData, it never gets called > directly by the app, only from FlushOnNetworkThread or onDataSent when previous > set of buffers has flushed. I learned from Andrei that we should re-use routine where possible during the review of the Android version of this change. It looks like we can combine SendFlushingWriteData() with the logic in FlushOnNetworkThread(). In OnDataSent(), we can invoke FlushOnNetworkThread() directly. FlushOnNetworkThread() will send pending data and/or pending flush data accordingly. I think it might be a cleaner interface that way.
https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:313: if (write_state_ != WAITING_FOR_FLUSH) { On 2016/06/10 19:00:01, xunjieli wrote: > On 2016/06/10 18:36:36, mef wrote: > > On 2016/06/10 16:30:50, xunjieli wrote: > > > Flush() can be called anytime after OnStreamReady() (even after stream is > > > destroyed), so we need to handle that. > > > > > > In FlushOnNetworkThread, maybe check that write_state_ == WAITING_FOR_FLUSH, > > if > > > not, just accumulate the pending write data in the flushing write data? > > > > > > Suggest combining this method with FlushOnNetworkThread() method. It might > be > > > slightly easier to reason. > > > > SendFlushingWriteData is similar to nativeWritevData, it never gets called > > directly by the app, only from FlushOnNetworkThread or onDataSent when > previous > > set of buffers has flushed. > > I learned from Andrei that we should re-use routine where possible during the > review of the Android version of this change. It looks like we can combine > SendFlushingWriteData() with the logic in FlushOnNetworkThread(). In > OnDataSent(), we can invoke FlushOnNetworkThread() directly. > FlushOnNetworkThread() will send pending data and/or pending flush data > accordingly. I think it might be a cleaner interface that way. In other words, iI am wondering if we can get rid of SendFlushingWriteData(), and move the logic to FlushOnNetworkThread(). The SendFlushingWriteData() method name is a bit unclear, and it seems that it is doing what FlushOnNetworkThread() should do. If it is, making it into a separate method might be confusing.
https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:306: if (!sending_write_data_) On 2016/06/10 19:00:01, xunjieli wrote: > Can this check be just "if (write_state_ != WRITING)"? Done. https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:313: if (write_state_ != WAITING_FOR_FLUSH) { On 2016/06/10 19:00:01, xunjieli wrote: > On 2016/06/10 18:36:36, mef wrote: > > On 2016/06/10 16:30:50, xunjieli wrote: > > > Flush() can be called anytime after OnStreamReady() (even after stream is > > > destroyed), so we need to handle that. > > > > > > In FlushOnNetworkThread, maybe check that write_state_ == WAITING_FOR_FLUSH, > > if > > > not, just accumulate the pending write data in the flushing write data? > > > > > > Suggest combining this method with FlushOnNetworkThread() method. It might > be > > > slightly easier to reason. > > > > SendFlushingWriteData is similar to nativeWritevData, it never gets called > > directly by the app, only from FlushOnNetworkThread or onDataSent when > previous > > set of buffers has flushed. > > I learned from Andrei that we should re-use routine where possible during the > review of the Android version of this change. It looks like we can combine > SendFlushingWriteData() with the logic in FlushOnNetworkThread(). In > OnDataSent(), we can invoke FlushOnNetworkThread() directly. I don't think we can though. FlushOnNetworkThread moves all pending write data into flushing_write_data_ and SendFlushingWriteData sends flushing_write_data_ if any. We don't want to flush pending data from onDataSent as it maybe incomplete (e.g. app written 2 buffers out of 5 and hasn't called Flush yet). > FlushOnNetworkThread() will send pending data and/or pending flush data > accordingly. I think it might be a cleaner interface that way.
Thanks, PTAL. https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/60001/components/cronet/ios/c... components/cronet/ios/cronet_bidirectional_stream.cc:313: if (write_state_ != WAITING_FOR_FLUSH) { On 2016/06/10 19:11:28, mef wrote: > On 2016/06/10 19:00:01, xunjieli wrote: > > On 2016/06/10 18:36:36, mef wrote: > > > On 2016/06/10 16:30:50, xunjieli wrote: > > > > Flush() can be called anytime after OnStreamReady() (even after stream is > > > > destroyed), so we need to handle that. > > > > > > > > In FlushOnNetworkThread, maybe check that write_state_ == > WAITING_FOR_FLUSH, > > > if > > > > not, just accumulate the pending write data in the flushing write data? > > > > > > > > Suggest combining this method with FlushOnNetworkThread() method. It might > > be > > > > slightly easier to reason. > > > > > > SendFlushingWriteData is similar to nativeWritevData, it never gets called > > > directly by the app, only from FlushOnNetworkThread or onDataSent when > > previous > > > set of buffers has flushed. > > > > I learned from Andrei that we should re-use routine where possible during the > > review of the Android version of this change. It looks like we can combine > > SendFlushingWriteData() with the logic in FlushOnNetworkThread(). In > > OnDataSent(), we can invoke FlushOnNetworkThread() directly. > I don't think we can though. FlushOnNetworkThread moves all pending write data > into flushing_write_data_ and SendFlushingWriteData sends flushing_write_data_ > if any. > > We don't want to flush pending data from onDataSent as it maybe incomplete (e.g. > app written 2 buffers out of 5 and hasn't called Flush yet). > > > FlushOnNetworkThread() will send pending data and/or pending flush data > > accordingly. I think it might be a cleaner interface that way. > Per offline discussion this is current behavior on Android, and should be ok, so I've changed iOS implementation to match.
https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:149: DCHECK_EQ(read_state_, STARTED); nit: the expected state should be the first arg. DCHECK_EQ(STARTED, read_state_); https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:296: if (flushing_write_data_) { I think it is slightly confusing that |flushing_write_data_| and |pending_write_data_| can be in three states: (1) null, (2) not-null but empty, (3) not-null and not empty. It might be easier if we have two std::vector<WrappedBufferWithSize>, which can only be empty or not empty, but never null. Can create a WrappedBufferWithSize which is a subclass of net::WrappedBuffer, but with a size. WDYT?
https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:149: DCHECK_EQ(read_state_, STARTED); On 2016/06/10 20:31:37, xunjieli wrote: > nit: the expected state should be the first arg. > DCHECK_EQ(STARTED, read_state_); Hrm, I think that's true for ASSERT_EQ, but I see DCHECK_EQ both ways. I don't mind changing though. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:296: if (flushing_write_data_) { On 2016/06/10 20:31:37, xunjieli wrote: > I think it is slightly confusing that |flushing_write_data_| and > |pending_write_data_| can be in three states: (1) null, (2) not-null but empty, > (3) not-null and not empty. > > It might be easier if we have two std::vector<WrappedBufferWithSize>, which can > only be empty or not empty, but never null. Can create a WrappedBufferWithSize > which is a subclass of net::WrappedBuffer, but with a size. > > WDYT? That's an interesting obeservation. With FlushOnNetworkThread doing moving data intp flushing vector anyway, I wonder whether we actually need 2 separate lists. I'll think about it.
https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:149: DCHECK_EQ(read_state_, STARTED); On 2016/06/10 21:15:14, mef wrote: > On 2016/06/10 20:31:37, xunjieli wrote: > > nit: the expected state should be the first arg. > > DCHECK_EQ(STARTED, read_state_); > > Hrm, I think that's true for ASSERT_EQ, but I see DCHECK_EQ both ways. I don't > mind changing though. I have been doing that way. But if there are places in the code base that use it in the other way, then I am not sure. Looking at the header file, DCHECK_EQ does put the variable in the second argument: https://cs.chromium.org/chromium/src/base/logging.h?rcl=0&l=746.
https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/BUILD.gn:56: ios_framework_bundle("cronet_framework") { Where do we specify such things like 'xcode_settings' to choose whether debug symbols should be stripped and dsym files generated? https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/BUILD.gn:148: copy("cronet_package_copy") { Do we need this target for the dynamic framework? It depends on "cronet_framework" target. I would expect that it should package the framework properly. We copy other resources (like VERSION) in package_ios.py. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:149: DCHECK_EQ(read_state_, STARTED); On 2016/06/10 22:22:09, xunjieli wrote: > On 2016/06/10 21:15:14, mef wrote: > > On 2016/06/10 20:31:37, xunjieli wrote: > > > nit: the expected state should be the first arg. > > > DCHECK_EQ(STARTED, read_state_); > > > > Hrm, I think that's true for ASSERT_EQ, but I see DCHECK_EQ both ways. I don't > > mind changing though. > > I have been doing that way. But if there are places in the code base that use it > in the other way, then I am not sure. Looking at the header file, DCHECK_EQ does > put the variable in the second argument: > https://cs.chromium.org/chromium/src/base/logging.h?rcl=0&l=746. Found this one: https://bugs.chromium.org/p/chromium/issues/detail?id=58409#c13. The order of DCHECK_EQ arguments is "expected", "actual". https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:263: DCHECK(environment_->IsOnNetworkThread()); Can we add DCHECK here to check that write_end_of_stream_ is 'false'? Maybe we should call onFailed()? https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:274: pending_write_data_->write_buffer_list.push_back(write_buffer); Can we create a method inside PendingWriteData struct and combine write_buffer_list.push_back() and write_buffer_len_list.push_back() in this method? E.g., pending_write_data_->AppendBuffer(write_buffer, buffer_size). I think it will make it more object-oriented. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:297: std::move(pending_write_data_->write_buffer_list.begin(), Maybe a new method in PendingWriteData struct? E.g., flushing_write_data_->MoveDataBuffers(pending_write_data_). https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:307: if ((write_state_ == WRITING) || !flushing_write_data_) I think the content of flushing_write_data_ can never be NULL here. Should we add DCHECK instead? https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:310: sending_write_data_.swap(flushing_write_data_); Should we add DCHECK that the content of sending_write_data_ is NULL? I assume that should always be true here. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.h (right): https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.h:59: void disable_auto_flush(bool disable_auto_flush) { I remember we discussed to get rid of disable_auto_flush() method and make flush() mandatory. Is it still here for backward compatibility? https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.h:64: void delay_headers_until_flush(bool delay_headers_until_flush) { Should we use camelcase for the sake of API consistency even though the function is "cheap"? I think the style guide gives the choice. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.h:89: bool Flush(); What does the return boolean mean? Is it possible that the flush fails? https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.h:134: struct PendingWriteData { |sending_write_data_|, |flushing_write_data_| and |pending_write_data_| are using this structure. Should we rename it to something like |DataBuffers| since the structure by itself is not pinned to any particular state or usage.
Thanks, PTAL. I think we can get rid of separate flushing_write_data_. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/BUILD.gn:56: ios_framework_bundle("cronet_framework") { On 2016/06/13 21:21:56, kapishnikov wrote: > Where do we specify such things like 'xcode_settings' to choose whether debug > symbols should be stripped and dsym files generated? That's a good question, I don't know yet. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/BUILD.gn:148: copy("cronet_package_copy") { On 2016/06/13 21:21:56, kapishnikov wrote: > Do we need this target for the dynamic framework? It depends on > "cronet_framework" target. I would expect that it should package the framework > properly. We copy other resources (like VERSION) in package_ios.py. Also good question. I think we need to generate license file as part of gn build, but package copy doesn't have to be a part of it. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:149: DCHECK_EQ(read_state_, STARTED); On 2016/06/13 21:21:57, kapishnikov wrote: > On 2016/06/10 22:22:09, xunjieli wrote: > > On 2016/06/10 21:15:14, mef wrote: > > > On 2016/06/10 20:31:37, xunjieli wrote: > > > > nit: the expected state should be the first arg. > > > > DCHECK_EQ(STARTED, read_state_); > > > > > > Hrm, I think that's true for ASSERT_EQ, but I see DCHECK_EQ both ways. I > don't > > > mind changing though. > > > > I have been doing that way. But if there are places in the code base that use > it > > in the other way, then I am not sure. Looking at the header file, DCHECK_EQ > does > > put the variable in the second argument: > > https://cs.chromium.org/chromium/src/base/logging.h?rcl=0&l=746. > > Found this one: https://bugs.chromium.org/p/chromium/issues/detail?id=58409#c13. > The order of DCHECK_EQ arguments is "expected", "actual". Done. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:263: DCHECK(environment_->IsOnNetworkThread()); On 2016/06/13 21:21:57, kapishnikov wrote: > Can we add DCHECK here to check that write_end_of_stream_ is 'false'? Maybe we > should call onFailed()? Done. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:274: pending_write_data_->write_buffer_list.push_back(write_buffer); On 2016/06/13 21:21:57, kapishnikov wrote: > Can we create a method inside PendingWriteData struct and combine > write_buffer_list.push_back() and write_buffer_len_list.push_back() in this > method? E.g., > pending_write_data_->AppendBuffer(write_buffer, buffer_size). > > I think it will make it more object-oriented. Done. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:296: if (flushing_write_data_) { On 2016/06/10 21:15:14, mef wrote: > On 2016/06/10 20:31:37, xunjieli wrote: > > I think it is slightly confusing that |flushing_write_data_| and > > |pending_write_data_| can be in three states: (1) null, (2) not-null but > empty, > > (3) not-null and not empty. > > > > It might be easier if we have two std::vector<WrappedBufferWithSize>, which > can > > only be empty or not empty, but never null. Can create a WrappedBufferWithSize > > which is a subclass of net::WrappedBuffer, but with a size. > > > > WDYT? > > That's an interesting obeservation. With FlushOnNetworkThread doing moving data > intp flushing vector anyway, I wonder whether we actually need 2 separate lists. > I'll think about it. Done. I think. :) https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:297: std::move(pending_write_data_->write_buffer_list.begin(), On 2016/06/13 21:21:57, kapishnikov wrote: > Maybe a new method in PendingWriteData struct? E.g., > flushing_write_data_->MoveDataBuffers(pending_write_data_). I think we don't need to separate pending_write_data from flusing_write_data as we always flush everything flushed before explicit Flush or onDataSent. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:307: if ((write_state_ == WRITING) || !flushing_write_data_) On 2016/06/13 21:21:56, kapishnikov wrote: > I think the content of flushing_write_data_ can never be NULL here. Should we > add DCHECK instead? Done. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:310: sending_write_data_.swap(flushing_write_data_); On 2016/06/13 21:21:57, kapishnikov wrote: > Should we add DCHECK that the content of sending_write_data_ is NULL? I assume > that should always be true here. Done. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.h (right): https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.h:59: void disable_auto_flush(bool disable_auto_flush) { On 2016/06/13 21:21:57, kapishnikov wrote: > I remember we discussed to get rid of disable_auto_flush() method and make > flush() mandatory. Is it still here for backward compatibility? Yes. I think we should keep it optional for compatibility / ease of transition. It is still present in Android as well. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.h:64: void delay_headers_until_flush(bool delay_headers_until_flush) { On 2016/06/13 21:21:57, kapishnikov wrote: > Should we use camelcase for the sake of API consistency even though the function > is "cheap"? I think the style guide gives the choice. I don't have strong preference, but I think Chromium doesn't use CamelCase for simple getters/setters. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.h:89: bool Flush(); On 2016/06/13 21:21:57, kapishnikov wrote: > What does the return boolean mean? Is it possible that the flush fails? Good point, changed to void. https://codereview.chromium.org/2050483002/diff/140001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.h:134: struct PendingWriteData { On 2016/06/13 21:21:57, kapishnikov wrote: > |sending_write_data_|, |flushing_write_data_| and |pending_write_data_| are > using this structure. Should we rename it to something like |DataBuffers| since > the structure by itself is not pinned to any particular state or usage. Done.
https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:204: for (scoped_refptr<net::IOBuffer> buffer : Does const & work here? I am not sure doing this way will add an extra ref. https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:210: if (!pending_write_data_->Empty()) { I am not exactly sure if we should auto flush |pending_write_data_|. Andrei, what do you think? https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:309: request_headers_sent_ = true; Maybe move this line inside of "if (!request_headers_sent)" block. https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.h (right): https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.h:150: std::vector<scoped_refptr<net::IOBuffer>> write_buffer_list; Since we have AppendBuffer, these member variables probably should be private. We don't want two ways of inserting elements right? Maybe add accessors for write_buffer_list and write_buffer_list_len https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... File components/cronet/ios/cronet_c_for_grpc.h (right): https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/cronet_c_for_grpc.h:153: CRONET_EXPORT int cronet_bidirectional_stream_disable_auto_flush( Why do these two methods return an int? should we make them void, so callers don't need to check the value? https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... File components/cronet/ios/test/cronet_bidirectional_stream_test.mm (right): https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/test/cronet_bidirectional_stream_test.mm:109: bool flush; Maybe add a brief comment on what |flush| is. https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/test/cronet_bidirectional_stream_test.mm:114: DISALLOW_COPY_AND_ASSIGN(WriteData); include macros.h? https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/test/cronet_bidirectional_stream_test.mm:243: test->write_data.pop_front(); Suggest that we also check |data| is the same as the write data that is in front of the queue. https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/test/cronet_bidirectional_stream_test.mm:261: static void on_succeded_callback(cronet_bidirectional_stream* stream) { maybe check test->write_data is now empty (we have received all on_write_completed callbacks).
https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:210: if (!pending_write_data_->Empty()) { Can it result in flushing data that were written but not actually flushed by the client? E.g., if the client calls "write", "flush", "write" while the first write is still in progress, will the second write be automatically flushed?
https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:210: if (!pending_write_data_->Empty()) { On 2016/06/14 14:29:34, xunjieli wrote: > I am not exactly sure if we should auto flush |pending_write_data_|. Andrei, > what do you think? Sorry Helen, I didn't see your comment, I think we should not autoflush |pending_write_data_| since it introduces non-deterministic behavior. E.g. let's say a developer forgot to add flush but ran the tests on a slow networks. Everything seems to work. Later, when the app is deployed on a faster network, it can start failing or show flaky behavior. I think we should avoid this type of scenarios.
Thanks, PTAL. https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:204: for (scoped_refptr<net::IOBuffer> buffer : On 2016/06/14 14:29:34, xunjieli wrote: > Does const & work here? I am not sure doing this way will add an extra ref. Done. https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:210: if (!pending_write_data_->Empty()) { On 2016/06/14 14:50:25, kapishnikov wrote: > On 2016/06/14 14:29:34, xunjieli wrote: > > I am not exactly sure if we should auto flush |pending_write_data_|. Andrei, > > what do you think? > > Sorry Helen, I didn't see your comment, I think we should not autoflush > |pending_write_data_| since it introduces non-deterministic behavior. E.g. let's > say a developer forgot to add flush but ran the tests on a slow networks. > Everything seems to work. Later, when the app is deployed on a faster network, > it can start failing or show flaky behavior. I think we should avoid this type > of scenarios. Done. https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:309: request_headers_sent_ = true; On 2016/06/14 14:29:34, xunjieli wrote: > Maybe move this line inside of "if (!request_headers_sent)" block. Done. https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.h (right): https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.h:150: std::vector<scoped_refptr<net::IOBuffer>> write_buffer_list; On 2016/06/14 14:29:34, xunjieli wrote: > Since we have AppendBuffer, these member variables probably should be private. > We don't want two ways of inserting elements right? Maybe add accessors for > write_buffer_list and write_buffer_list_len Done. https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... File components/cronet/ios/cronet_c_for_grpc.h (right): https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/cronet_c_for_grpc.h:153: CRONET_EXPORT int cronet_bidirectional_stream_disable_auto_flush( On 2016/06/14 14:29:35, xunjieli wrote: > Why do these two methods return an int? should we make them void, so callers > don't need to check the value? Done. https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... File components/cronet/ios/test/cronet_bidirectional_stream_test.mm (right): https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/test/cronet_bidirectional_stream_test.mm:109: bool flush; On 2016/06/14 14:29:35, xunjieli wrote: > Maybe add a brief comment on what |flush| is. Done. https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/test/cronet_bidirectional_stream_test.mm:114: DISALLOW_COPY_AND_ASSIGN(WriteData); On 2016/06/14 14:29:35, xunjieli wrote: > include macros.h? Done. https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/test/cronet_bidirectional_stream_test.mm:243: test->write_data.pop_front(); On 2016/06/14 14:29:35, xunjieli wrote: > Suggest that we also check |data| is the same as the write data that is in front > of the queue. Hrm, is it different than ASSERT_EQ(test->write_data.front()->buffer.c_str(), data); above? https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/test/cronet_bidirectional_stream_test.mm:261: static void on_succeded_callback(cronet_bidirectional_stream* stream) { On 2016/06/14 14:29:35, xunjieli wrote: > maybe check test->write_data is now empty (we have received all > on_write_completed callbacks). Done.
Thanks. I will take a look later today. https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... File components/cronet/ios/test/cronet_bidirectional_stream_test.mm (right): https://codereview.chromium.org/2050483002/diff/180001/components/cronet/ios/... components/cronet/ios/test/cronet_bidirectional_stream_test.mm:243: test->write_data.pop_front(); On 2016/06/14 19:54:31, mef wrote: > On 2016/06/14 14:29:35, xunjieli wrote: > > Suggest that we also check |data| is the same as the write data that is in > front > > of the queue. > > Hrm, is it different than ASSERT_EQ(test->write_data.front()->buffer.c_str(), > data); above? Acknowledged. Sorry, I missed that part.
One question on |write_end_of_stream_| below. I just realized I am on network bug triage duty today and tomorrow, so I might not be able to get back to this until Friday. If Andrei is happy, please go ahead with the CL, I will make corresponding changes on the Java side on Friday :) https://codereview.chromium.org/2050483002/diff/200001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/200001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:53: std::back_inserter(target->write_buffer_list)); Is this different from: "target->write_buffer_list.push_back(std::move(write_buffer_list))" ? If we do so, we don't need to call Clear() right? since std::move will remove the elements from |write_buffer_list| and make it empty? https://codereview.chromium.org/2050483002/diff/200001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:223: if (write_end_of_stream_) There might be a slight problem with setting WRITING_DONE based on |write_end_of_stream_| here. We might have data written to the pending queue with a FIN flag, so |write_end_of_stream_| is true. But we might not have flushed that buffer yet when we received a callback for a previous buffer. We might need to change WriteBuffers to make note of which buffer has a FIN flag. https://codereview.chromium.org/2050483002/diff/200001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:334: sending_write_data_.swap(flushing_write_data_); Should we prefer using std::move here since move semantics is now supported?
https://codereview.chromium.org/2050483002/diff/200001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/200001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:223: if (write_end_of_stream_) On 2016/06/15 14:07:19, xunjieli wrote: > There might be a slight problem with setting WRITING_DONE based on > |write_end_of_stream_| here. We might have data written to the pending queue > with a FIN flag, so |write_end_of_stream_| is true. But we might not have > flushed that buffer yet when we received a callback for a previous buffer. We > might need to change WriteBuffers to make note of which buffer has a FIN flag. Nice catch, Helen. I think this should be addressed since it can result in firing of delegate_->OnSucceeded() and reseting the stream before all data are actually flushed.
Thanks, PTAL. https://codereview.chromium.org/2050483002/diff/200001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/200001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:53: std::back_inserter(target->write_buffer_list)); On 2016/06/15 14:07:19, xunjieli wrote: > Is this different from: > "target->write_buffer_list.push_back(std::move(write_buffer_list))" ? > > If we do so, we don't need to call Clear() right? since std::move will remove > the elements from |write_buffer_list| and make it empty? Hrm, TBH I'm not very familiar with std::move yet. std::vector::push_back only accepts one element, not another vector. std::vector::insert takes begin / end iterators, so I'm not sure how to express that in combination with std::move. https://codereview.chromium.org/2050483002/diff/200001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:223: if (write_end_of_stream_) On 2016/06/15 16:37:26, kapishnikov wrote: > On 2016/06/15 14:07:19, xunjieli wrote: > > There might be a slight problem with setting WRITING_DONE based on > > |write_end_of_stream_| here. We might have data written to the pending queue > > with a FIN flag, so |write_end_of_stream_| is true. But we might not have > > flushed that buffer yet when we received a callback for a previous buffer. We > > might need to change WriteBuffers to make note of which buffer has a FIN flag. > > Nice catch, Helen. I think this should be addressed since it can result in > firing of delegate_->OnSucceeded() and reseting the stream before all data are > actually flushed. Done. Good point. It wasn't critical when we were flushing all pending writes, but it is now. https://codereview.chromium.org/2050483002/diff/200001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:334: sending_write_data_.swap(flushing_write_data_); On 2016/06/15 14:07:19, xunjieli wrote: > Should we prefer using std::move here since move semantics is now supported? Done, although swapping 2 pointers seems more performant and just as easy to reason about.
LGTM
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2050483002/240001
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Coalesce small buffers into single QUIC packet in GRPC on iOS. Added 'disable_auto_flush' and 'delay_request_headers_until_flush' to cronet_c_for_grpc API. BUG=601972 ========== to ========== [Cronet] Coalesce small buffers into single QUIC packet in GRPC on iOS. Added 'disable_auto_flush' and 'delay_request_headers_until_flush' to cronet_c_for_grpc API. BUG=601972 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Coalesce small buffers into single QUIC packet in GRPC on iOS. Added 'disable_auto_flush' and 'delay_request_headers_until_flush' to cronet_c_for_grpc API. BUG=601972 ========== to ========== [Cronet] Coalesce small buffers into single QUIC packet in GRPC on iOS. Added 'disable_auto_flush' and 'delay_request_headers_until_flush' to cronet_c_for_grpc API. BUG=601972 Committed: https://crrev.com/62d0ba2a00cd3eeca6afa9edc572829a711410b8 Cr-Commit-Position: refs/heads/master@{#400009} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/62d0ba2a00cd3eeca6afa9edc572829a711410b8 Cr-Commit-Position: refs/heads/master@{#400009}
Message was sent while issue was closed.
https://codereview.chromium.org/2050483002/diff/240001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/240001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:225: MaybeOnSucceded(); For future CL: I think MaybeOnSucceded() can go inside the 'if' body.
Message was sent while issue was closed.
https://codereview.chromium.org/2050483002/diff/240001/components/cronet/ios/... File components/cronet/ios/cronet_bidirectional_stream.cc (right): https://codereview.chromium.org/2050483002/diff/240001/components/cronet/ios/... components/cronet/ios/cronet_bidirectional_stream.cc:336: sending_write_data_->lengths(), write_end_of_stream_); While I was doing the same change on Android side, I noticed that there is a problem with using |write_end_of_stream_| here. The bug is currently present in both Android and iOS. If we have written EndOfStream, but we have non-empty |pending_write_data_|. We can incorrectly set EndOfStream = true for |flushing_write_data_|. To reproduce, let's say we have 3 buffers to write. Write all three buffers at once just after stream is ready, but only call flush after the second write. stream.write(buffer1, /*eos*=/false); stream.write(buffer2, /*eos*=/false); stream.flush(); stream.write(buffer3, /*eos*=/true); SendFlushingWriteData() for the first flush() can happen after the final write, so |write_end_of_stream_| is true. SendFlushingWriteData() will end up sending two buffers with EndOfStream = true. The next time we try to flush the last buffer, it will be just no-op because writing is done :( I reproduced on Android side, and I am working on a fix.
Message was sent while issue was closed.
On 2016/06/20 15:24:24, xunjieli wrote: > https://codereview.chromium.org/2050483002/diff/240001/components/cronet/ios/... > File components/cronet/ios/cronet_bidirectional_stream.cc (right): > > https://codereview.chromium.org/2050483002/diff/240001/components/cronet/ios/... > components/cronet/ios/cronet_bidirectional_stream.cc:336: > sending_write_data_->lengths(), write_end_of_stream_); > While I was doing the same change on Android side, I noticed that there is a > problem with using |write_end_of_stream_| here. The bug is currently present in > both Android and iOS. > > If we have written EndOfStream, but we have non-empty |pending_write_data_|. We > can incorrectly set EndOfStream = true for |flushing_write_data_|. > > To reproduce, let's say we have 3 buffers to write. Write all three buffers at > once just after stream is ready, but only call flush after the second write. > stream.write(buffer1, /*eos*=/false); > stream.write(buffer2, /*eos*=/false); > stream.flush(); > stream.write(buffer3, /*eos*=/true); > > SendFlushingWriteData() for the first flush() can happen after the final write, > so |write_end_of_stream_| is true. SendFlushingWriteData() will end up sending > two buffers with EndOfStream = true. The next time we try to flush the last > buffer, it will be just no-op because writing is done :( > > I reproduced on Android side, and I am working on a fix. Good catch! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
