Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697833003:
[Cronet] Add isDone() check in CronetBidirectionalStream#onWritevCompleted (Closed)
Description[Cronet] Add isDone() check in CronetBidirectionalStream#onWritevCompleted
This CL adds a isDone() to check to onWritevCompleted() so we can
avoid calling into native adapter if it's gone.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
BUG=692168
Review-Url: https://codereview.chromium.org/2697833003
Cr-Commit-Position: refs/heads/master@{#450826}
Committed: https://chromium.googlesource.com/chromium/src/+/a2f61739b0f7cfecd118bcf0e3cb5fe6f72da6b3
Patch Set 1 #
Total comments: 1
Patch Set 2 : address comments #
Total comments: 6
Patch Set 3 : Get rid of ugly test #Patch Set 4 : added test per suggestion #
Total comments: 1
Patch Set 5 : new test #Patch Set 6 : re-enable check #
Total comments: 6
Messages
Total messages: 26 (11 generated)
Description was changed from ========== [Cronet] Change CronetBidirectionalStream#isDone() to check native adapter. This CL changes CronetBidirectionalStream#isDone() to check validity of native adapter only. There are reports with crashes happening in WriteData() just after the adapter is destroyed. This CL makes so that we won't call into native code if the adapter is gone. ========== to ========== [Cronet] Change CronetBidirectionalStream#isDone() to check native adapter. This CL changes CronetBidirectionalStream#isDone() to check validity of native adapter only. There are reports with crashes happening in WriteData() just after the adapter is destroyed. This CL makes so that we won't call into native code if the adapter is gone. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
xunjieli@chromium.org changed reviewers: + mef@chromium.org
Misha: PTAL
https://codereview.chromium.org/2697833003/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/impl/CronetBidirectionalStream.java (right): https://codereview.chromium.org/2697833003/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/impl/CronetBidirectionalStream.java:549: mWriteState = State.WAITING_FOR_FLUSH; I think we should add the check if (isDoneLocked()) { return; } here instead, because it is possible that adapter was destroyed while Writev was running on network thread.
Description was changed from ========== [Cronet] Change CronetBidirectionalStream#isDone() to check native adapter. This CL changes CronetBidirectionalStream#isDone() to check validity of native adapter only. There are reports with crashes happening in WriteData() just after the adapter is destroyed. This CL makes so that we won't call into native code if the adapter is gone. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [Cronet] Change CronetBidirectionalStream#isDone() to check native adapter. This CL changes CronetBidirectionalStream#isDone() to check validity of native adapter only. There are reports with crashes happening in WriteData() just after the adapter is destroyed. This CL makes so that we won't call into native code if the adapter is gone. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=692168 ==========
Description was changed from ========== [Cronet] Change CronetBidirectionalStream#isDone() to check native adapter. This CL changes CronetBidirectionalStream#isDone() to check validity of native adapter only. There are reports with crashes happening in WriteData() just after the adapter is destroyed. This CL makes so that we won't call into native code if the adapter is gone. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=692168 ========== to ========== [Cronet] Add isDone() check in CronetBidirectionalStream#onWritevCompleted This CL adds a isDone() to check to onWritevCompleted() so we can avoid calling into native adapter if it's gone. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=692168 ==========
Thanks! PTAL. I am not sure how to write a more deterministic regression test. The one I added looks a bit ugly...
https://codereview.chromium.org/2697833003/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetBidirectionalStream.java (right): https://codereview.chromium.org/2697833003/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetBidirectionalStream.java:549: if (isDoneLocked()) return; Just checking: Does this syntax conform to coding guidelines? https://codereview.chromium.org/2697833003/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetBidirectionalStream.java:565: postTaskToExecutor(new OnWriteCompletedRunnable(buffer, I think if you use direct executor and cancel stream from very first OnWriteCompleted callback (which will cause native adapter to be set to 0) then the next completion of nativeWritevData should consistently trigger the issue.
https://codereview.chromium.org/2697833003/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetBidirectionalStream.java (right): https://codereview.chromium.org/2697833003/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetBidirectionalStream.java:549: if (isDoneLocked()) return; On 2017/02/14 21:02:47, mef wrote: > Just checking: Does this syntax conform to coding guidelines? Yes, this is listed under "Use Standard Brace Style" on http://source.android.com/source/code-style.html https://codereview.chromium.org/2697833003/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetBidirectionalStream.java:565: postTaskToExecutor(new OnWriteCompletedRunnable(buffer, On 2017/02/14 21:02:47, mef wrote: > I think if you use direct executor and cancel stream from very first > OnWriteCompleted callback (which will cause native adapter to be set to 0) then > the next completion of nativeWritevData should consistently trigger the issue. Right, but how do we reliably trigger nativeWritevData?
https://codereview.chromium.org/2697833003/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetBidirectionalStream.java (right): https://codereview.chromium.org/2697833003/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetBidirectionalStream.java:549: if (isDoneLocked()) return; On 2017/02/14 21:14:34, xunjieli wrote: > On 2017/02/14 21:02:47, mef wrote: > > Just checking: Does this syntax conform to coding guidelines? > > Yes, this is listed under "Use Standard Brace Style" on > http://source.android.com/source/code-style.html Cool, thanks for the reference! https://codereview.chromium.org/2697833003/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetBidirectionalStream.java:565: postTaskToExecutor(new OnWriteCompletedRunnable(buffer, On 2017/02/14 21:14:34, xunjieli wrote: > On 2017/02/14 21:02:47, mef wrote: > > I think if you use direct executor and cancel stream from very first > > OnWriteCompleted callback (which will cause native adapter to be set to 0) > then > > the next completion of nativeWritevData should consistently trigger the issue. > > > Right, but how do we reliably trigger nativeWritevData? Would the way you had it in the test (write / flush, write / flush, etc) work? That way once first write completes there will be another portion of flush data ready.
Neat. Using a direct executor avoids the race. Thanks for the suggestion! PTAL.
sorry, that didn't fixed the issue. I mistook an assert failure for the crash. Let me think about it more
https://codereview.chromium.org/2697833003/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/2697833003/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:417: callback.addWriteData(dummyString, true); Should there be a third writeData? First write after onStreamReady. Second write flushed right after. Third write from onWriteCompleted.
PTAL. Thanks! https://codereview.chromium.org/2697833003/diff/100001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/2697833003/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:404: public void testCancelWhileWriteDataPending() throws Exception { Misha, so I came up with this test where we cancel in onReadCompleted() when there are writes pending. This test crashes with the same callstack almost 100% of the times locally (though it didn't crash on the bot). In order to receive onWritevCompleted() after an async cancel(), we need to keep the send side busy. I achieved so by continuously writing in onWriteCompleted() user callback without a FIN flag. This is much more reliably than doing a predefined number of writes. Here we are more confident because when the first Callback#onReadCompleted() is received, there will be additional native onWritevCompleted()s.
Patchset #7 (id:120001) has been deleted
lgtm https://codereview.chromium.org/2697833003/diff/100001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/2697833003/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:404: public void testCancelWhileWriteDataPending() throws Exception { On 2017/02/15 15:51:43, xunjieli wrote: > Misha, so I came up with this test where we cancel in onReadCompleted() when > there are writes pending. This test crashes with the same callstack almost 100% > of the times locally (though it didn't crash on the bot). > > In order to receive onWritevCompleted() after an async cancel(), we need to keep > the send side busy. I achieved so by continuously writing in onWriteCompleted() > user callback without a FIN flag. This is much more reliably than doing a > predefined number of writes. Here we are more confident because when the first > Callback#onReadCompleted() is received, there will be additional native > onWritevCompleted()s. Great, thanks for making this work! https://codereview.chromium.org/2697833003/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:420: stream.cancel(); Nice, this is happening while write is in flight. https://codereview.chromium.org/2697833003/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:437: ByteBuffer dummyData = ByteBuffer.allocateDirect(data.length); nit: I think you can do dummyData.put('x') in the loop above and skip data.
https://codereview.chromium.org/2697833003/diff/100001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/2697833003/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:437: ByteBuffer dummyData = ByteBuffer.allocateDirect(data.length); On 2017/02/15 18:20:14, mef wrote: > nit: I think you can do dummyData.put('x') in the loop above and skip data. If I put a single byte each time, flip() will only flip the last put() right? If so, I need to update dummyData's position and limit. That is probably less readable?
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2697833003/diff/100001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java (right): https://codereview.chromium.org/2697833003/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:437: ByteBuffer dummyData = ByteBuffer.allocateDirect(data.length); On 2017/02/15 20:32:46, xunjieli wrote: > On 2017/02/15 18:20:14, mef wrote: > > nit: I think you can do dummyData.put('x') in the loop above and skip data. > > If I put a single byte each time, flip() will only flip the last put() right? > If so, I need to update dummyData's position and limit. That is probably less > readable? Sorry, my bad. flip() does reset the position to 0. Locally it complains that "error: no suitable method found for put(char)" for dummyData.put('x'). I need to cast 'x' to byte. I will keep the intermediate step for readability. Thanks!
The CQ bit was unchecked by xunjieli@chromium.org
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487194342050890,
"parent_rev": "bfa0420d49cb8d5c301ef378db26078a50f40248", "commit_rev":
"a2f61739b0f7cfecd118bcf0e3cb5fe6f72da6b3"}
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Add isDone() check in CronetBidirectionalStream#onWritevCompleted This CL adds a isDone() to check to onWritevCompleted() so we can avoid calling into native adapter if it's gone. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=692168 ========== to ========== [Cronet] Add isDone() check in CronetBidirectionalStream#onWritevCompleted This CL adds a isDone() to check to onWritevCompleted() so we can avoid calling into native adapter if it's gone. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=692168 Review-Url: https://codereview.chromium.org/2697833003 Cr-Commit-Position: refs/heads/master@{#450826} Committed: https://chromium.googlesource.com/chromium/src/+/a2f61739b0f7cfecd118bcf0e3cb... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/a2f61739b0f7cfecd118bcf0e3cb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
