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 headers sent with forwarded requests #22

Merged
merged 2 commits into from
Oct 20, 2015
Merged

Fix headers sent with forwarded requests #22

merged 2 commits into from
Oct 20, 2015

Conversation

akshayjshah
Copy link
Contributor

Per the TChannel spec, the minimum header is 2 bytes. Without this change, forwarded requests always time out.

Per the TChannel spec, the minimum header is 2 bytes.
@thanodnl
Copy link
Contributor

Thanks for the find!

There is a problem though. These two bytes were there before, but it crashed a json example in the repository. PR: #14

This indicates to us we need to dig deeper into how support all TChannel protocols in HandleOrForward.

If this fix solves the problems you are having you can use this feature branch till we have a final fix for this. But merging in a branch which is known to fail somewhere else is not preferred.

@akshayjshah
Copy link
Contributor Author

Hmmm, I didn't realize that this would break the JSON forwarding.

Is it feasible to merge this into master and fix the JSON forwarding afterwards? As far as I know, I'm the only person using this library in production, and I'd really prefer to deploy the master branch. To me, it seems sensible to have master support production, with any non-prod features in development branches.

@prashantv
Copy link
Contributor

I think the simplest fix given the current API is to specify the headers as part of the HandleOrForward. That way the caller can choose to pass nil for json or 0,0 bytes for Thrift.

This should be temporary till we come up with a good API to forward the request directly from TChannel.

@prashantv
Copy link
Contributor

Take a look at #24

@akshayjshah
Copy link
Contributor Author

@prashantv and @thanodnl - the request format is in scope at the point we write the headers. I've amended this PR to switch on the request type, so this should now handle JSON and Thrift calls properly. The default is the current nil behavior, so it shouldn't change behavior for any non-Thrift callers.

Is this a workable solution?

@thanodnl
Copy link
Contributor

👍 lgtm

akshayjshah added a commit that referenced this pull request Oct 20, 2015
Fix headers sent with forwarded requests
@akshayjshah akshayjshah merged commit 16162fd into master Oct 20, 2015
@thanodnl thanodnl deleted the header branch September 13, 2016 14:26
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

3 participants