Skip to content

Do not fail requests when side channel file cannot be used#396

Merged
vbuberen merged 4 commits into
ChuckerTeam:developfrom
MiSikora:side-channel-file-failure
Jul 28, 2020
Merged

Do not fail requests when side channel file cannot be used#396
vbuberen merged 4 commits into
ChuckerTeam:developfrom
MiSikora:side-channel-file-failure

Conversation

@MiSikora

@MiSikora MiSikora commented Jul 26, 2020

Copy link
Copy Markdown
Contributor

📄 Context

There is an open issue - #394.

📝 Changes

TeeSource guards against failures to use files. When FileNotFoundException occurs when opening a stream it is reported back to the caller and no payload side streaming happens. Also, any IO failures during interception are now logged.

🚫 Breaking

No.

🛠️ How to test

Not really sure how to reproduce this manually without a convoluted setup. Maybe @CodeBreak524 can confirm that their issue is fixed with a dependency from my branch on JitPack (com.github.MiSikora.chucker:library:side-channel-file-failure-SNAPSHOT). I don't really feel that it is necessary as the description in #394 is quite clear and it is now covered with tests.

⏱️ Next steps

Closes #394.

To be honest I wonder now about the necessity to write to a file instead of to the memory. Responses need to be loaded to a memory at some point anyway.

private fun readResponseBuffer(responseBody: File, isGzipped: Boolean): Buffer {
val bufferedSource = Okio.buffer(Okio.source(responseBody))
val source = if (isGzipped) {
GzipSource(bufferedSource)
} else {
bufferedSource
}
return Buffer().apply { source.use { writeAll(it) } }
}

I pushed for a file-based solution before because of parallel downloads but I didn't consider that the content of files needs to be read to the memory before it is saved to the DB. However, there is still an advantage of files that could be leveraged. Files could be saved to some non-volatile directory instead of cache and DB could just store handles to these files (processing still would be required as we need to decompress network streams). I see the pros and cons of both solutions and I'm open to suggestions.

@vbuberen vbuberen added the bug Something isn't working label Jul 27, 2020
@vbuberen vbuberen added this to the 3.3.0 milestone Jul 27, 2020

@vbuberen vbuberen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for another great contribution.

@cortinico cortinico left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cortinico

Copy link
Copy Markdown
Member

However, there is still an advantage of files that could be leveraged. Files could be saved to some non-volatile directory instead of cache and DB could just store handles to these files (processing still would be required as we need to decompress network streams).

As you said, responses would have to be loaded in memory anyway (e.g. you have to show an image, parse the body of a JSON to split it in strings, etc). I feel that the file approach we have right now is way more robust than before, where we just loaded everything in memory with subsequent OOM errors.

I'll suggest that we stick to it and try to refine it following issue reports (as we're doing) 👍

@vbuberen

Copy link
Copy Markdown
Collaborator

Agree with @cortinico here that current approach looks pretty robust for now.

@vbuberen vbuberen merged commit feb287f into ChuckerTeam:develop Jul 28, 2020
@MiSikora MiSikora deleted the side-channel-file-failure branch July 28, 2020 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FileNotFoundException drops the request

3 participants