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

analytics RecordHit fixed #2000

Merged
merged 4 commits into from
Nov 29, 2018
Merged

analytics RecordHit fixed #2000

merged 4 commits into from
Nov 29, 2018

Conversation

dencoded
Copy link
Contributor

@dencoded dencoded commented Nov 28, 2018

added changes for #1996

Thus situation was happening when detailed analytics recording (copy of actual response in analytics record) is enabled.

The code in RecordHit was screwing up copyResponse.Body underlaying buffer by reseting its length to 0. So item put into cache had only headers without body and it was causing that error http: proxy error during body copy: unexpected EOF as we use reverse-proxy code to instantiate response from cache item value.

@dencoded dencoded requested a review from buger November 28, 2018 14:29
@buger
Copy link
Member

buger commented Nov 28, 2018

This is a tricky code, so I decided to write a test (added to this branch). And while EOF error was solved, RawResponse field not get recorded when cache is used.

@dencoded
Copy link
Contributor Author

Agree that it is tricky code, unfortunately I couldn't find better way to do a fix as the place when source Body buffer gets reset to len 0 is inside Go's standard library code.

According to empty empty RawResponse - I think this is because we don't pass response copy for cache hits here https://github.com/TykTechnologies/tyk/blob/master/mw_redis_cache.go#L266
I didn't change that peace and it looks like a legacy thing.

@dencoded
Copy link
Contributor Author

@buger after looking again at code history I am pretty sure we have introduced problem with EOF from cache and empty RawResponse here: #1778

so might worth to revise that merged PR and come up with better fix

@dencoded
Copy link
Contributor Author

I will look into that PR changes with potentially another fix

@dencoded
Copy link
Contributor Author

@buger PTAL

@buger
Copy link
Member

buger commented Nov 29, 2018

Nice! So much clearer.

@buger buger merged commit 5e01175 into master Nov 29, 2018
@buger buger deleted the global-cache-fix branch November 29, 2018 13:39
buger pushed a commit that referenced this pull request Nov 29, 2018
added changes for #1996

Thus situation was happening when detailed analytics recording (copy of actual response in analytics record) is enabled.

The code in RecordHit was screwing up copyResponse.Body underlaying buffer by reseting its length to 0. So item put into cache had only headers without body and it was causing that error `http: proxy error during body copy: unexpected EOF` as we use reverse-proxy code to instantiate response from cache item value.
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.

None yet

2 participants