Properly handle unexhausted network responses #288

MiSikora · 2020-03-26T13:56:33Z

📄 Context

There is an issue #287

📝 Changes

TeeSource now reports partially read responses as failures. This way ChuckerInterceptor does not try to process them. The bytes are still available in the side channel file if in the future we'd like to process these bytes.

I also wrapped reading from file in a try/catch expression for a good measure because of the volatile nature of a phone storage.

🚫 Breaking

No.

🛠️ How to test

Unit tests are written. @oakkitten might confirm whether this fixes their issue using Chucker version from PR via JitPack.

⏱️ Next steps

Closes #287.

MiSikora · 2020-03-26T14:07:38Z

@oakkitten Could you check if this PR fixes the issue that you experience? You can try with a version from JitPack - https://jitpack.io/#MiSikora/chucker/corrupted-files-read-fix-SNAPSHOT.

vbuberen

Would like to see the feedback of the original issue reporter.

Hope he won't disappear, like in #250 :)

vbuberen · 2020-03-26T23:21:06Z

library/src/main/java/com/chuckerteam/chucker/internal/support/TeeSource.kt

@@ -27,6 +27,7 @@ internal class TeeSource(
    private var totalBytesRead = 0L
    private var reachedLimit = false
    private var upstreamFailed = false
+    private var upstreamConsumed = false


Can we change the name here to something more usual for boolean variables - isUpstreamConsumed?
Also, just saw that 2 variables above could also be renamed.

oakkitten · 2020-03-27T14:46:36Z

the exception is gone, but there's another problem which is probably closely related to #209

it seems that in order for Chucker to display the transferred contents, it has to finish reading the input stream, and by finishing reading the stream i mean that reader.read() should return -1. if i close the stream when it has been only partially read, or if it was read fully but the trailing read() has not been called, the response body remains empty.

it would be nice—for clarity—to never have the body part of the response tab empty, or at least only have it empty only if there can't be a body, as with a response to HEAD. consider having “no body” or “waiting for the app to read the body...” there

p.s. i'm using Chucker as network interceptor

MiSikora · 2020-03-27T14:51:07Z

But what would you expect to see if bytes are missing from the body? I agree that there should be better UX where user is informed that body was not fully read or whatever but it is not possible to format data that is not complete without doing some ugly workarounds. And when you consider compression headers/footers it gets even uglier (which very often will be the case for network interceptors).

oakkitten · 2020-03-27T14:56:43Z

something along...

    <html>
        <head>
            <title>nice titl

only a part of the document has been read

e: or in case of non-text data,

15 bytes of 300 have been read

...android will show partially downloaded images properly, so if it's an image, you can display it the usual way

MiSikora · 2020-03-27T15:02:18Z

As I mentioned, this gets really ugly very quickly to handle when you consider things that I mentioned. I mean, you can create an issue and give it a try if there is a need for it.

oakkitten · 2020-03-27T15:18:35Z

i plugged Chucker as an application level interceptor and it still doesn't seem to be displaying the body... application level interceptors aren't dealing with compression, so these should be able to display partial content just fine, shouldn't they? and even if it's compressed, isn't it easy unzipping on the fly?

even if all this is hard to accomplish, i still think that some message is necessary. empty body is just confusing

i will gladly open another issue when this is merged

p.s. thanks for all the the work! 🙇‍♀️

MiSikora · 2020-03-27T15:23:48Z

Yes, it wouldn't work for application level interceptor as well. I mentioned network level interceptor because this is where most of the compression happens and it makes it that much harder. Unzipping on the fly is also not that trivial. Doable but will require well-thought solution in order to support different types of compression. Right now only gzip is supported but there are plans for i.e. Brotli compression support.

Parsing plain text body wouldn't be too bad. We would just read it as a string and be done with that. Formatting it gets much trickier because most of the formatters require properly formed bodies in order to format them. They can be lenient to some extent but we can't be really sure what will be handled by them. I might be wrong here and maybe there are solutions that can handle this already.

But I 100% agree about some feedback message when body was not fully read or exceeded capacity limit.

vbuberen · 2020-03-27T17:07:51Z

@oakkitten
Regarding empty state - you could check our list of issues first, since it is already there.
As to your suggestion with partially read body - you are welcome to implement it.

@MiSikora
Thanks for your PR. I am merging it and let's finish #250 to do a release before moving further with other stuff.

oakkitten · 2020-03-27T18:06:54Z

@MiSikora i see, thanks for your input!

@vbuberen wow ok. so sorry i didn't realize that the issue of missing body in the response tab, which in my tests seemed to be directly tied to partially fetched data cases, is an existing issue. and i don't know kotlin so sadly i won't be able to contribute to Chucker's codebase in the nearest future.

vbuberen · 2020-03-27T18:29:38Z

In such case I would like to ask you to fill a feature request with as much info as possible to speed up its consideration and possible implementation.

* Remove scroll flags (#210) * Fix/gradle properties (#211) * Allow Gradle parallel build * Fix version name * Fix for curl command (#214) * Fix R8 minification crash on TransactionViewModel creation. (#219) * Big resources renaming (#216) * Fix clear action crash when application is dead (#222) * Fix for crash on Save transaction action (#221) * Show warning is transaction is null, fix crash in Save action * Uncomment sample transactions * Replace mulptiple returning with multiple throw due to detekt issue * Add message into IOException, update string for request being not ready * Fix for NPE in NotificationHelper (#223) * Add additional check fo transaction being not null before getting its notificationText * Extract transaction item from transactionBuffer * ViewModel refactoring (#220) * Update ViewModel dependency, refactor TransactionViewModel * Dependencies clean up * Switch to ViewModel on the main screen * Fix depleting bytes from the response. (#226) * Use HttpUrl instead of Uri for parsing URL data. * Do not read image sources directly from the response. * Simplify gzip logic. * Move gzip checks from IoUtils to OkHttpUtils. * Remove unused 'Response.hasBody()' extension. * Update library/src/main/java/com/chuckerteam/chucker/internal/support/OkHttpUtils.kt * Revert resource renaming (#227) * Revert renaming * Add changelgos for 3.1.2 (#230) * Add missing section to release 3.1.1 and 3.1.2 (#232) * Update Github templates (#235) * Update templates * Remove redundant dot * Remove default `no` from the checkbox * Switch to platform defined HTTP code constants (#238) * Add instruction for checkbox selection (#237) * Fix HttpHeader serialization when obfuscation is enabled (#240) * Update README (#243) * Add Action to validate the Gradle Wrapper (#245) * Gradle to 6.2 (#247) * Do not refresh transaction when it is not being affected. (#246) * Do not refresh transaction when it is not being affected. * Use correct null-aware comparison for HttpTransaction. * Add switching between encoded and decoded URL formats. (#233) * Add switching between encoded and decoded URL formats. * Make URL encoding transaction specific. * Change test name for upcoming #244 PR. * Use LiveDataRecord for combineLatest tests. * Properly switch encoding and decoding URL. * Show encoding icon only when it is viable. * Add encoded URL samples to HttpBinClient. * Avoid using 'this' scoping mechanism for URL. * Fix typo in feature request comment (#251) * RTL Support (#208) * Remove ltr forcing and replace ScrollView in Overview * Replace Overview layout, add rtl support for it * Add textDirection and textAlignment property for API 21+ * Fix host textview constraints * Replace android:src with app:srcCompat * Update ids for layouts to avoid clashes * Update Material components to stable * Remove supportsRTL tag from Manifest, update Gradle plugin * Styles update * Remove supportsRTL from library manifest * Revert usage of supportVectorDrawables to avoid crashes on APIs 16-19 due to notifications icons * Fix lint issue with vector drawable * Response search fixes (#256) * Fix response search to be case insensitive * Add minimum required symbols * Fix invalid options menu in response fragment * Feature/tls info (#252) * Add UI for TLS info * Implement logic for retrieving TLS info * Address code review feedback * Switch views to ViewBinding (#253) * Switch to ViewBinding in activities * Switch to ViewBinding in ErrorsAdapter, add formattedDate field into Throwable models * Transaction adapter switch to ViewBinding * Remove variable for formatted date from models * Switch to ViewBinding in TransactionPayloadAdapter * Switch to ViewBinding in TransactionPaayload and TransactionOverviewFragments * Switch list fragments to ViewBinding * Fix link for tutorial opening * Rename views * Address code review feedback * Hide SSL field if isSSL is null * Libs update (#260) * Update tools versions * JUnit update * Feature/truth (#258) * Add Truth, update part of test classes * Convert other tests to use Truth, fix date parser test * Add Truth assertions to FormatUtilsTest, fix ktlint issue * Update assertions to a proper syntax * Add missing ThrowableViewModel (#257) * Add Error ViewModel, update title in TransactionActivity in onCreate * Switch from errors to throwable naming to have a uniform naming style * Rename toolbar title * Migrating from Travis to GH Actions (#262) * Setup GH Actions * Run only on Linux * Remove Travis File * Run only gradlew directly * Update targetSDK and Kotlin (#264) * Add stale builds canceller (#265) * Add filters * Update Gradle wrapper validation workflow * Update pre-merge workflow * Fixed various Lints (#268) * fixed typos * fixed KDocs * Replace Travis badge with GH Actions badge (#269) * Remove redundant JvmName (#274) * Fix margins and paddings for payload items (#277) * Add selective interception. (#279) * Add selective interception. * Update README.md. * Align formatting in README with other points. * Avoid header name duplication. * Strip interception header also in the no-op library. * UX improvements (#275) * Add icon for non-https transactions * Update secondary color to be more contrast * Simplify protocol resources setting * Add tests to format utils (#281) * add tests to format utils * fixes after code review * formatting fix Co-authored-by: adammasyk <adam.masyk@programisci.eu> * format utils test refactor (#285) * format utils test refactor * share text test refactor * Migrate to Kotlin Coroutines (#270) * Add coroutine as a dependency in build.gradle * Migrate AsyncTasks to kotlin coroutines * Migrate executors with the coroutines in repositories * Multi cast upstream response for Chucker consumption. (#267) * Multi cast response upstream for Chucker consumption. * Read buffer prefix before potentially gunzipping it. * Inform Chucker about unprocessed responses. * Simplify multi casting logic. * Move read offset to a variable. * Inline one-line method. * Give better control over TeeSource read results. * Add documentation to TeeSource. * Close side channel when capacity is exceeded. Co-authored-by: Volodymyr Buberenko <vbuberen@users.noreply.github.com> * Remove unnecessary mock method. (#289) * removed redundant Gson configurations (#290) * increased test coverage for format utils (#291) Co-authored-by: Karthik R <karthr@paypal.com> * added few test cases for json formatting (#295) * Properly handle unexhausted network responses (#288) * Handle properly not consumed upstream body. * Handle IO issues while reading from file. * Update dependencies (#296) * Update depencies * Update OkHttp to 3.12.10 * Handle empty and missing response bodies. (#250) * Add failing test cases. * Remove unused const. * Gzip response body if it was gunzipped. * Add test cases for regular bodies in Chucker. * Fix rule formatting. * Use proper name for application interceptor. * Return original response downstream. * Account for no content with gzip encoding. * Use Truth for Chucker tests. * Honor empty plaintext bodies. * Revert changes to HttpBinClient. * Update library/src/test/java/com/chuckerteam/chucker/ChuckerInterceptorDelegate.kt * Update library/src/main/java/com/chuckerteam/chucker/internal/support/OkHttpUtils.kt Co-authored-by: Nicola Corti <corti.nico@gmail.com> Co-authored-by: Volodymyr Buberenko <vbuberen@users.noreply.github.com> * Add hasFixed size to RecyclerViews (#297) * Detekt to 1.7.4 (#299) * Revert "Add selective interception. (#279)" (#300) This reverts commit d14ed64. * Prepare 3.2.0 (#298) * Update versions info * Update Changelog * Fix links and update date Co-authored-by: Michał Sikora <michalsikora90@gmail.com> Co-authored-by: Nicola Corti <corti.nico@gmail.com> Co-authored-by: Sergey Chelombitko <119192+technoir42@users.noreply.github.com> Co-authored-by: Michał Sikora <me@mehow.io> Co-authored-by: Hitanshu Dhawan <hitanshudhawan1996@gmail.com> Co-authored-by: adammasyk <masiol91@gmail.com> Co-authored-by: adammasyk <adam.masyk@programisci.eu> Co-authored-by: Nikhil Chaudhari <nikhyl777@gmail.com> Co-authored-by: karthik rk <karthik.rk18@gmail.com> Co-authored-by: Karthik R <karthr@paypal.com>

MiSikora added 2 commits March 26, 2020 14:42

Handle properly not consumed upstream body.

4db1678

Handle IO issues while reading from file.

4c420b1

MiSikora changed the title ~~Corrupted files read fix~~ Properly recognise not exhausted network responses Mar 26, 2020

MiSikora changed the title ~~Properly recognise not exhausted network responses~~ Properly recognise unexhausted network responses Mar 26, 2020

MiSikora changed the title ~~Properly recognise unexhausted network responses~~ Properly handle unexhausted network responses Mar 26, 2020

vbuberen approved these changes Mar 26, 2020

View reviewed changes

CR fixes.

e349fe7

vbuberen merged commit 9bafb00 into ChuckerTeam:develop Mar 27, 2020

vbuberen added this to the 3.2.0 milestone Mar 27, 2020

MiSikora deleted the corrupted-files-read-fix branch April 6, 2020 17:14

MiSikora mentioned this pull request Jul 2, 2020

Response body is not shown when stream is not fully received #370

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly handle unexhausted network responses #288

Properly handle unexhausted network responses #288

MiSikora commented Mar 26, 2020 •

edited

Loading

MiSikora commented Mar 26, 2020 •

edited

Loading

vbuberen left a comment

vbuberen Mar 26, 2020

oakkitten commented Mar 27, 2020

MiSikora commented Mar 27, 2020 •

edited

Loading

oakkitten commented Mar 27, 2020 •

edited

Loading

MiSikora commented Mar 27, 2020 •

edited

Loading

oakkitten commented Mar 27, 2020

MiSikora commented Mar 27, 2020 •

edited

Loading

vbuberen commented Mar 27, 2020

oakkitten commented Mar 27, 2020

vbuberen commented Mar 27, 2020

Properly handle unexhausted network responses #288

Properly handle unexhausted network responses #288

Conversation

MiSikora commented Mar 26, 2020 • edited Loading

📄 Context

📝 Changes

🚫 Breaking

🛠️ How to test

⏱️ Next steps

MiSikora commented Mar 26, 2020 • edited Loading

vbuberen left a comment

Choose a reason for hiding this comment

vbuberen Mar 26, 2020

Choose a reason for hiding this comment

oakkitten commented Mar 27, 2020

MiSikora commented Mar 27, 2020 • edited Loading

oakkitten commented Mar 27, 2020 • edited Loading

MiSikora commented Mar 27, 2020 • edited Loading

oakkitten commented Mar 27, 2020

MiSikora commented Mar 27, 2020 • edited Loading

vbuberen commented Mar 27, 2020

oakkitten commented Mar 27, 2020

vbuberen commented Mar 27, 2020

MiSikora commented Mar 26, 2020 •

edited

Loading

MiSikora commented Mar 26, 2020 •

edited

Loading

MiSikora commented Mar 27, 2020 •

edited

Loading

oakkitten commented Mar 27, 2020 •

edited

Loading

MiSikora commented Mar 27, 2020 •

edited

Loading

MiSikora commented Mar 27, 2020 •

edited

Loading