Multi cast upstream response for Chucker consumption. #267

MiSikora · 2020-03-06T22:50:16Z

📄 Context

There are couple of issues. #218 #263

📝 Changes

I changed mostly ChuckerInterceptor. It uses now custom source for responses that multicasts to end consumer and to a temporary file from which Chucker can read once the consumption is done by the end consumer.

📎 Related PR

There will be conflicts with #250. I would suggest to merge this one first. It will be easier to account for changes in #250 than other way around.

🚫 Breaking

No. I changed the constructor but I made it internal and added another one that is public and compatible with the old one.

🛠️ How to test

I don't know how to manually test it reliably. Make a request for a really big file and save it on a device you should get an OOM with the old code.

Theoretically user still can get errors if there is not enough disk space but I see it as an edge case with limiting the response size.

⏱️ Next steps

No further steps expected. Maybe we should create an issue for doing the same with requests? Also, maybe Chucker should process these files on its custom Executor.

Closes #218
Closes #263

MiSikora · 2020-03-06T23:16:51Z

Hmmm… I see now that for some reason deflate request is not getting processed. Will dig into it.

MiSikora · 2020-03-06T23:28:40Z

I know that testing is lacking for the things that I fixed in 4751cf0 and 03990f6 but it would be much easier for me to write tests for it in #250 after this one is merged. Would it be ok with you guys?

# Conflicts: # library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt

vbuberen · 2020-03-11T09:33:30Z

Hey @MiSikora. Yeah, probably, it would be easier.
The only moment here is that I am not a big fan of this approach with having some temp files. I would like to stick to approach with in-memory handling of the request/response, since storage is slower than RAM and because I would prefer Chucker to leave as low footprint on the device/emulator as possible.

Another moment that disturbs me that currently all the changes in the core of the library (classes like ChuckerInterceptor) are done mainly without any distinct vision or plan, so each individual pushes his own. I am against it, because it makes project development unpredictable and the library itself being a box of different approaches instead of following some defined vision.

MiSikora · 2020-03-11T15:23:23Z

Hi @vbuberen. This is a proposal implementation of a thing discussed here. Although it is a very basic approach and a first step and not full file-based solution. #242 (comment)

MiSikora · 2020-03-18T20:20:38Z

@vbuberen @cortinico
Any further thoughts on this? I'll gladly discuss my reasoning behind this implementation.

File processing seems better to me because it has lower impact on a device. If responses were multicasted to an in-memory buffers, then it might still result in OOM for multiple request. This is not very likely, but plausible. What is more realistic scenario is increased request size to some amount that would not fit in the memory. This would result in the OOM for sure. With multiple parallel requests it is even worse.

I'm not sure I understand the argument about the footprint. Chucker already leaves a footprint due to the database. Files are temporary and are removed anyway. Manual calls to delete() will almost always remove the files. If not, they will be removed after some time due to being in the cache directory. And finally they will be removed after uninstalling the app.

Processing is theoretically slower but disk IO is nothing compared to the network IO.

Additional benefit of using files (apart from more or less solving OOM issues) is that in the future Chucker could skip storing responses as blobs in the database directly and rely only on file handles. Not sure if it is a direction that you would like to go, but in my opinion it would be beneficial.

What do you think?

vbuberen · 2020-03-19T10:25:38Z

Ok, I agree with your point. I just didn't think about big payloads properly before and yeah, temp files are the best solution. Also, I should admit that we can always rework the solution at any moment if we ever need it.
So, green light from me. Going to review this PR.

# Conflicts: # library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt

vbuberen · 2020-03-19T13:18:25Z

library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt

@@ -15,6 +19,7 @@ import okhttp3.Response
 import okhttp3.ResponseBody
 import okio.Buffer
 import okio.GzipSource
+import okio.Okio

 private const val MAX_BLOB_SIZE = 1000_000L


Seems that this const is unused, because the same is present in companion object.

I didn't touch it because it is removed in another PR but I'll remove it here after we find a solution to the main issue in this PR. :)

vbuberen · 2020-03-19T13:20:11Z

library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt

+        return multiCastResponseBody(response) { cachedResponseFile ->
+            val buffer = cachedResponseFile?.let { readResponseBuffer(it, response.isGzipped) }
+            cachedResponseFile?.delete()
+            if (buffer != null) processResponseBody(response, buffer, transaction)


This whole block starting with return is too hard to read. Let's rewrite it in more simple way without putting multiple functions in return.

vbuberen · 2020-03-19T13:29:07Z

library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt

+        )
+
+        return response.newBuilder()
+            .body(ResponseBody.create(contentType, contentLength, Okio.buffer(teeSource)))


I insist on returning the original Response object further in chain without any builders, cloning, etc.
We had enough side effects and issues with providing processed or somehow changed objects.
And while I see that here we should get the same object I still don't like that it is recreated.
In some issues we already mentioned that Chucker should be as transparent as possible and this is possible only if we do copy only for Chucker processing without anything like that.

I don't see how it would be possible to stream original bytes to two sinks at the same time without replacing the response. This is a mechanism that is used under the hood by OkHttp for caching (just a simpler version). It is also recommended way by Square (square/okio#186). Do you have any other approach in mind that would solve the problems addressed by this?

library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt

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

vbuberen · 2020-03-19T13:34:57Z

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

+
+        totalBytesRead += bytesRead
+        if (!reachedLimit && totalBytesRead <= readBytesLimit) {
+            sink.copyTo(sideStream.buffer(), sink.size() - bytesRead, bytesRead)


Why don't you move sink.size() - bytesRead into a variable?

vbuberen · 2020-03-19T13:35:27Z

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

+    }
+
+    override fun timeout(): Timeout {
+        return upstream.timeout()


What about removing return?

…ceptor.kt Co-Authored-By: Volodymyr Buberenko <[email protected]>

…/TeeSource.kt Co-Authored-By: Volodymyr Buberenko <[email protected]>

cortinico

Left some comments. Having more comments around would also be beneficial 👍
I generally like the approach.

On one side I also realised that the maxContentLength constructor parameters was used to truncate ONLY the request and not the response. This PR fixes this.
I'm still wondering if this approach is a bit over engineered as truncating the responses will probably solve the OOM as well.

cortinico · 2020-03-21T21:13:19Z

library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt

-        return processedResponse
+        processResponseMetadata(response, transaction)
+        return multiCastResponseBody(response) { cachedResponseFile ->
+            val buffer = cachedResponseFile?.let { readResponseBuffer(it, response.isGzipped) }


Can probably be rewritten to:

Suggested change

val buffer = cachedResponseFile?.let { readResponseBuffer(it, response.isGzipped) }

cachedResponseFile

?.let { readResponseBuffer(it, response.isGzipped) }

?.let { processResponseBody(response, it, transaction) }

cachedResponseFile?.delete()

cortinico · 2020-03-21T21:28:40Z

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

+import okio.Source
+import okio.Timeout
+
+internal class TeeSource(


Please leave some comments around as otherwise this code will harder to maintain.
For example explaining that you're writing a special prefix on every file, etc...

cortinico · 2020-03-21T21:42:41Z

library/src/test/java/com/chuckerteam/chucker/internal/support/TeeSourceTest.kt

+        val testSource = TestSource(10_000)
+        val downstream = Buffer()
+
+        val teeSource = TeeSource(testSource, testFile)


readBytesLimit = 9_999 is missing here.

cortinico · 2020-03-21T21:43:37Z

library/src/test/java/com/chuckerteam/chucker/internal/support/TeeSourceTest.kt

+    }
+
+    private class TestSource(contentLength: Int = 1_000) : Source {
+        val content = ByteString.of(*Random.nextBytes(contentLength))


Please specify the type of content here as it's inferred as a platform type.

MiSikora · 2020-03-22T02:16:55Z

I will make a CR fixes some time later to focus discussion on things that are not yet clear to me.

On one side I also realised that the maxContentLength constructor parameters was used to truncate ONLY the request and not the response. This PR fixes this.
I'm still wondering if this approach is a bit over engineered as truncating the responses will probably solve the OOM as well.

I don't think this is really viable. One approach might be to rely on Content-Length header but it is not always available and responses can be from bytes to megabytes. We could skip such responses but then we will have some false negatives or false positives depending on how they'll be treated.

Another approach might to try to do something like this:

val response = chain.proceed(chain.request())
val length = response.body()?.source()?.buffer?.size() ?: return response
if (length <= maxContentLength) {
  // Process response.
}
return response

This, however, also does not work. Buffer size is always 0 because it has not been read from at this point and will be some time in the future when the user consumes their network response and process it or whatever they do with it.

The only reason that we can check it right now is because of this piece of code.

chucker/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt

Line 166 in c343002

val buffer = Buffer().apply { responseSource.use { writeAll(it) } }

And this is the culprit here because it loads the whole response into the memory and potentially leads to OOM.

MiSikora · 2020-03-22T10:37:42Z

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

+    interface Callback {
+        /**
+         * Called when the upstream was successfully copied to the [file].
+         */
+        fun onSuccess(file: File)
+
+        /**
+         * Called when there was an issue while copying bytes to the [file].
+         *
+         * It might occur due to an exception thrown while reading bytes or due to exceeding capacity limit.
+         */
+        fun onFailure(exception: IOException, file: File)
+    }


I decided to go with a callback instead of magic headers. It gives type safety and is more extensible. In the future it is possible to add onContentLengthAvailable(length: Long) method in order to resolve #209.

MiSikora · 2020-03-22T10:42:17Z

I applied most of the suggestions from the CR. One thing still stands - replacing the response. As I mentioned I don't know any other way that would solve the problems addressed by this PR. One thing I'd like to highlight is about the volatile nature of replacing the response and issues that occurred in the past. It is reasonable to be hesitant to do this again, however previous code did not have any tests for ChuckerInterceptor. Now that they are being gradually added they should give a confidence in doing such changes. And with more tests to come in #250 it should have a good test coverage.

vbuberen · 2020-03-23T20:59:50Z

Sorry for taking that long. I just want to play around in attempt to not replace response, because I don't know possible approach at the moment. Will try to do tomorrow.

MiSikora · 2020-03-23T21:01:07Z

No worries. Take your time, I just want to provide as much information as possible. I know it is rather a big change so it's best to consider it carefully.

vbuberen

Let's give it a try and see if we get any issues.

vbuberen · 2020-03-25T16:07:30Z

If no other commits planned here, I will merge it.

MiSikora · 2020-03-25T16:21:13Z

Yes, it is ready to go. I'll update #250 soon

* 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 <[email protected]> * 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 <[email protected]> * Remove unnecessary mock method. (#289) * removed redundant Gson configurations (#290) * increased test coverage for format utils (#291) Co-authored-by: Karthik R <[email protected]> * 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 <[email protected]> Co-authored-by: Volodymyr Buberenko <[email protected]> * 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 <[email protected]> Co-authored-by: Nicola Corti <[email protected]> Co-authored-by: Sergey Chelombitko <[email protected]> Co-authored-by: Michał Sikora <[email protected]> Co-authored-by: Hitanshu Dhawan <[email protected]> Co-authored-by: adammasyk <[email protected]> Co-authored-by: adammasyk <[email protected]> Co-authored-by: Nikhil Chaudhari <[email protected]> Co-authored-by: karthik rk <[email protected]> Co-authored-by: Karthik R <[email protected]>

MiSikora added 2 commits March 6, 2020 23:45

Multi cast response upstream for Chucker consumption.

6e2406d

Read buffer prefix before potentially gunzipping it.

4751cf0

MiSikora changed the title ~~Multi cast response upstream for Chucker consumption.~~ Multi cast upstream response for Chucker consumption. Mar 6, 2020

Code formatting.

0756d3f

Inform Chucker about unprocessed responses.

03990f6

MiSikora mentioned this pull request Mar 8, 2020

Payload size isn't correct for some transactions #209

Closed

MiSikora added 3 commits March 8, 2020 15:46

Simplify multi casting logic.

624341b

Fix typo.

19c9a39

Merge branch 'develop' into tee

14096fa

# Conflicts: # library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt

MiSikora added 3 commits March 19, 2020 12:10

Merge branch 'develop' into tee

f64b429

# Conflicts: # library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt

Fix a typo.

400ffbc

Lint fixes.

617ea7c

vbuberen requested changes Mar 19, 2020

View reviewed changes

MiSikora and others added 2 commits March 19, 2020 14:58

Update library/src/main/java/com/chuckerteam/chucker/api/ChuckerInter…

5772e9d

…ceptor.kt Co-Authored-By: Volodymyr Buberenko <[email protected]>

Update library/src/main/java/com/chuckerteam/chucker/internal/support…

f0a8ee6

…/TeeSource.kt Co-Authored-By: Volodymyr Buberenko <[email protected]>

cortinico requested changes Mar 21, 2020

View reviewed changes

MiSikora added 4 commits March 22, 2020 11:34

Remove unused constant.

cdce1f0

Move read offset to a variable.

5b019d9

Inline one-line method.

8f0344b

Give better control over TeeSource read results.

6cebad6

MiSikora commented Mar 22, 2020

View reviewed changes

Add documentation to TeeSource.

190d1aa

MiSikora added 2 commits March 22, 2020 14:12

Fix a typo.

0e9c832

Close side channel when capacity is exceeded.

2f0e139

MiSikora mentioned this pull request Mar 23, 2020

Handle empty and missing response bodies. #250

Merged

vbuberen approved these changes Mar 25, 2020

View reviewed changes

vbuberen merged commit 2b2e402 into ChuckerTeam:develop Mar 25, 2020

cortinico added this to the 3.2.0 milestone Mar 25, 2020

cortinico added the enhancement New feature or improvement to the library label Mar 25, 2020

MiSikora deleted the tee branch March 25, 2020 17:21

MiSikora mentioned this pull request Oct 28, 2020

Cannot access database on the main thread #487

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi cast upstream response for Chucker consumption. #267

Multi cast upstream response for Chucker consumption. #267

MiSikora commented Mar 6, 2020 •

edited

Loading

MiSikora commented Mar 6, 2020

MiSikora commented Mar 6, 2020 •

edited

Loading

vbuberen commented Mar 11, 2020

MiSikora commented Mar 11, 2020 •

edited

Loading

MiSikora commented Mar 18, 2020 •

edited

Loading

vbuberen commented Mar 19, 2020

vbuberen Mar 19, 2020

MiSikora Mar 19, 2020 •

edited

Loading

vbuberen Mar 19, 2020

vbuberen Mar 19, 2020

MiSikora Mar 19, 2020

vbuberen Mar 19, 2020

vbuberen Mar 19, 2020

cortinico left a comment

cortinico Mar 21, 2020

cortinico Mar 21, 2020

cortinico Mar 21, 2020

cortinico Mar 21, 2020

MiSikora commented Mar 22, 2020 •

edited

Loading

MiSikora Mar 22, 2020

MiSikora commented Mar 22, 2020 •

edited

Loading

vbuberen commented Mar 23, 2020

MiSikora commented Mar 23, 2020 •

edited

Loading

vbuberen left a comment

vbuberen commented Mar 25, 2020

MiSikora commented Mar 25, 2020

Multi cast upstream response for Chucker consumption. #267

Multi cast upstream response for Chucker consumption. #267

Conversation

MiSikora commented Mar 6, 2020 • edited Loading

📄 Context

📝 Changes

📎 Related PR

🚫 Breaking

🛠️ How to test

⏱️ Next steps

MiSikora commented Mar 6, 2020

MiSikora commented Mar 6, 2020 • edited Loading

vbuberen commented Mar 11, 2020

MiSikora commented Mar 11, 2020 • edited Loading

MiSikora commented Mar 18, 2020 • edited Loading

vbuberen commented Mar 19, 2020

Choose a reason for hiding this comment

MiSikora Mar 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cortinico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiSikora commented Mar 22, 2020 • edited Loading

Choose a reason for hiding this comment

MiSikora commented Mar 22, 2020 • edited Loading

vbuberen commented Mar 23, 2020

MiSikora commented Mar 23, 2020 • edited Loading

vbuberen left a comment

Choose a reason for hiding this comment

vbuberen commented Mar 25, 2020

MiSikora commented Mar 25, 2020

MiSikora commented Mar 6, 2020 •

edited

Loading

MiSikora commented Mar 6, 2020 •

edited

Loading

MiSikora commented Mar 11, 2020 •

edited

Loading

MiSikora commented Mar 18, 2020 •

edited

Loading

MiSikora Mar 19, 2020 •

edited

Loading

MiSikora commented Mar 22, 2020 •

edited

Loading

MiSikora commented Mar 22, 2020 •

edited

Loading

MiSikora commented Mar 23, 2020 •

edited

Loading