Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a bug where the response cache could be corrupted. #1102

Merged
merged 1 commit into from
Oct 27, 2014

Conversation

swankjesse
Copy link
Collaborator

When streaming a response, we copy data from our buffer to the cached file
on disk. Unfortunately we were copying N bytes from the front of the buffer
when we wanted N bytes from the back of the buffer.

Typically these are the same, but certain access patterns can cause them
to be different, corrpting the cached file on disk.

This was uncovered by migrating the cache tests from operating on
HttpURLConnection's API to our new API.

@swankjesse
Copy link
Collaborator Author

Code reviewer warning!

GitHub might not be showing the big changes to CacheTest, which is where the bulk of the test refactoring lives.

The UrlConnectionCacheTest.java change is just a mechanical file move.

// TODO source.copyTo(cacheBody, byteCount);
cacheBody.write(source.clone(), byteCount);
// TODO source.copyTo(cacheBody, source.size() - byteCount, byteCount)
Buffer sourceCopy = source.clone();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The bug!

@mescortes
Copy link
Contributor

nit: CacheTest.java line 1536: over 100 cols

@mescortes
Copy link
Contributor

btw I reviewed it on IJ since github won't help.

@mescortes
Copy link
Contributor

LGTM

When streaming a response, we copy data from our buffer to the cached file
on disk. Unfortunately we were copying N bytes from the front of the buffer
when we wanted N bytes from the back of the buffer.

Typically these are the same, but certain access patterns can cause them
to be different, corrpting the cached file on disk.

This was uncovered by migrating the cache tests from operating on
HttpURLConnection's API to our new API.
swankjesse added a commit that referenced this pull request Oct 27, 2014
Fix a bug where the response cache could be corrupted.
@swankjesse swankjesse merged commit a4c0d59 into master Oct 27, 2014
@swankjesse swankjesse deleted the jwilson_1025_fix_cache_bug branch November 5, 2014 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants