Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1657103002:
Remove unnecessary loop and quit in CronetChunkedOutputStream (Closed)
|
Created:
4 years, 10 months ago by xunjieli Modified:
4 years, 5 months ago Reviewers:
pauljensen CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove unnecessary loop and quit in CronetChunkedOutputStream
The MessageLoop.loop() in CronetChunkedOutputStream.close()
is potentially problematic. If the user closes the
OutputStream unintentionally when MessageLoop is not going
to be quit, it might wait forever. This removes the
corresponding unnecessary quit().
BUG=626653
Committed: https://crrev.com/578950706701e926c468cc1ab8128e94bd032e08
Cr-Commit-Position: refs/heads/master@{#404391}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address Comment #Messages
Total messages: 19 (12 generated)
Description was changed from ========== Remove unnecessary loop and quit in CronetChunkedOutputStream BUG= ========== to ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it will wait forever. This removes the corresponding unnecessary quit(). ==========
xunjieli@chromium.org changed reviewers: + pauljensen@chromium.org
Paul, PTAL.
Description was changed from ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it will wait forever. This removes the corresponding unnecessary quit(). ========== to ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it might wait forever. This removes the corresponding unnecessary quit(). ==========
lgtm though might be good to have a test or explain in the CL description when exactly a call to close() could have resulted in a hang. You should also clarify "The MessageLoop.loop() in CronetChunkedOutputStream" to "The MessageLoop.loop() in CronetChunkedOutputStream.close()" https://codereview.chromium.org/1657103002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java (right): https://codereview.chromium.org/1657103002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java:85: if (!mLastChunk) { I think we can get rid of this if-statement now.
Description was changed from ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it might wait forever. This removes the corresponding unnecessary quit(). ========== to ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream#close() is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it might wait forever. This removes the corresponding unnecessary quit(). ==========
Description was changed from ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream#close() is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it might wait forever. This removes the corresponding unnecessary quit(). ========== to ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream#close() is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it might wait forever. This removes the corresponding unnecessary quit(). ==========
Description was changed from ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream#close() is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it might wait forever. This removes the corresponding unnecessary quit(). ========== to ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream#close() is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it might wait forever. This removes the corresponding unnecessary quit(). BUG=626653 ==========
Description was changed from ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream#close() is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it might wait forever. This removes the corresponding unnecessary quit(). BUG=626653 ========== to ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream#close() is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it might wait forever. This removes the corresponding unnecessary quit(). BUG=626653 ==========
Description was changed from ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream#close() is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it might wait forever. This removes the corresponding unnecessary quit(). BUG=626653 ========== to ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream.close() is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it might wait forever. This removes the corresponding unnecessary quit(). BUG=626653 ==========
Done. Thanks! Since this API is getting more exercised now. I think it deserves a rewrite. I will land this now. Will add more testing in a follow-up CL. https://codereview.chromium.org/1657103002/diff/1/components/cronet/android/j... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java (right): https://codereview.chromium.org/1657103002/diff/1/components/cronet/android/j... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java:85: if (!mLastChunk) { On 2016/02/04 15:27:26, pauljensen wrote: > I think we can get rid of this if-statement now. Done.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org Link to the patchset: https://codereview.chromium.org/1657103002/#ps20001 (title: "Address Comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream.close() is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it might wait forever. This removes the corresponding unnecessary quit(). BUG=626653 ========== to ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream.close() is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it might wait forever. This removes the corresponding unnecessary quit(). BUG=626653 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream.close() is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it might wait forever. This removes the corresponding unnecessary quit(). BUG=626653 ========== to ========== Remove unnecessary loop and quit in CronetChunkedOutputStream The MessageLoop.loop() in CronetChunkedOutputStream.close() is potentially problematic. If the user closes the OutputStream unintentionally when MessageLoop is not going to be quit, it might wait forever. This removes the corresponding unnecessary quit(). BUG=626653 Committed: https://crrev.com/578950706701e926c468cc1ab8128e94bd032e08 Cr-Commit-Position: refs/heads/master@{#404391} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/578950706701e926c468cc1ab8128e94bd032e08 Cr-Commit-Position: refs/heads/master@{#404391} |
