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

Support of Go 1.7 http.Request's Context #251

Closed
wants to merge 1 commit into from
Closed

Support of Go 1.7 http.Request's Context #251

wants to merge 1 commit into from

Conversation

zirkome
Copy link

@zirkome zirkome commented Nov 4, 2016

This PR enable to use the Go 1.7 http.Request Context.

This feature is disable by default but can be enabled through the request_context parameter.

The usefulness of this is that we can now add middlewares and add metadata before calling gRPC services. I'm also using that as part of a distributed system (e.g. Zipkin) and need to pass metadata between HTTP and gRPC which can only be done through the same per-request context.Context.

If you enable the option there will be breaking changes.

@tmc
Copy link
Collaborator

tmc commented Nov 5, 2016

@kokaz thank you for your contribution -- can you document said breaking changes next to the parameter description in README.md?

@zirkome
Copy link
Author

zirkome commented Nov 7, 2016

@tmc I updated the README.md is that OK?

@zirkome
Copy link
Author

zirkome commented Nov 10, 2016

ping @tmc :)

@tmc
Copy link
Collaborator

tmc commented Nov 10, 2016

I'm uncomfortable with the amount of duplication involved here but really do like the idea.

What do you think about maintaining the surface api but preferring req.Context in Register{{$svc.GetName}}Handle?

@tmc
Copy link
Collaborator

tmc commented Nov 10, 2016

I'd like to keep the number of knobs low for simplicity's sake -- another option on the table would be dropping go<1.7 support. (I don't have great sense of the distribution of active go versions using this package).

@zirkome
Copy link
Author

zirkome commented Nov 11, 2016

@tmc I don't like this amount of duplication too, but that was the only solution that came to my mind to avoid direct breaking changes. I like the idea of dropping support for versions < go1.7 but like you said it might be a problem if the distribution of < go1.7 is too large. I don't know how we can deal with this. If we're going to support for versions > go1.7 I'd be happy to redo my PR to take this in account.

@zirkome
Copy link
Author

zirkome commented Nov 11, 2016

@tmc A thought that I had was to use build tags in order to prefer one API over the other but this involves moving templates in another files (e.g. template_pre17.go and template_go17.go). The gRPC-Go transport package has build tags for pre1.6, 1.6 and 1.7 (https://github.com/grpc/grpc-go/tree/master/transport)

@hatchan
Copy link

hatchan commented Nov 11, 2016

@kokaz The build tags need to be added to the generated code, right? As it doesn't matter which Go version was used to compile this binary.

The generated code could generate two files and then have build tags applied. But the biggest issue is the API signature of the generated code is different for both versions. Whereas the older on expects a context, the new code does not.

@zirkome
Copy link
Author

zirkome commented Nov 11, 2016

@hatchan This PR changed the signature of Register{{$svc.GetName}}Handle but in fact the best to do that would have be to just ignore the context with _. It will be part of the next iteration of this PR after we have a final decision on all that :)

@zirkome
Copy link
Author

zirkome commented Nov 17, 2016

@tmc what do you think?

@tmc
Copy link
Collaborator

tmc commented Nov 17, 2016

I think not changing the signature is the way to go there.

This will mean req.Context() will be a "context" not an "x/net/context" which has some less-than-friendly implications until upstream drops 1.6 support or we get aliasing.

It seems like the simplest thing would to just swap out that one particular line depending on the flag's presence vs wading into build tag complexity -- i think we can assume folks that opt into this flag are responsible adults that know what they're doing.

@zirkome
Copy link
Author

zirkome commented Nov 18, 2016

@tmc I changed the signature so that it won't break anything. For the std context vs x/net/context this won't break anything as it's just an interface and that x/net/context wraps around the std package when building with Go1.7.

Btw, the build seems to fail because of genswagger lint issue

@tmc
Copy link
Collaborator

tmc commented Nov 19, 2016

@kokaz what do you think about ditching the extra template and just doing the logic in a conditional in one template?

@tmc
Copy link
Collaborator

tmc commented Nov 20, 2016

@kokaz we discussed offline but just to drive the conversation forward, this is the sort of approach I had in mind: master...tmc:request_context

@zirkome
Copy link
Author

zirkome commented Nov 20, 2016

@tmc This remove a lot of the duplicate code I did, so I'm in favor to close this PR and use your work

@tmc
Copy link
Collaborator

tmc commented Nov 20, 2016

@kokaz great, opened another PR, let's move discussion there.

@tmc tmc closed this Nov 20, 2016
@zirkome zirkome deleted the use_request_context branch November 20, 2016 21:13
ithinker1991 pushed a commit to tronprotocol/grpc-gateway that referenced this pull request Apr 26, 2018
ithinker1991 added a commit to tronprotocol/grpc-gateway that referenced this pull request May 11, 2018
e92f847 Merge pull request grpc-ecosystem#46 from tronprotocol/rate_limit
e191aed add grpc return type:server busy
c98bb68 Merge pull request grpc-ecosystem#45 from tronprotocol/add_expiration
5abd2c0 fix spell error
76d2ac1 Merge pull request grpc-ecosystem#44 from tronprotocol/add-reason-enum
cf23e93 Merge pull request grpc-ecosystem#42 from tronprotocol/add_witness_committee
e9b0876 add the reason code
cc24179 Merge pull request grpc-ecosystem#43 from tronprotocol/add_expiration
5f9e263 add transaction expiration erro
3913331 fix merge conflict
bb1f7e0 add message Vote
4e0f34e Merge pull request grpc-ecosystem#40 from tronprotocol/proxy
7134384 minor change
ab77170 add witness and committee in account
8f7655a add error code for broadcast.
4bc508c fix build error.
a568eb8 add response code for return of broad_cast && fix error.
4dc6857 add response code for return of broad_cast.
df2ea4b Merge pull request grpc-ecosystem#41 from tronprotocol/add-reason-enum
54dac0c fix-reason
200ba66 proxy support GET method
0e65a14 Merge pull request grpc-ecosystem#39 from tronprotocol/evan-rm-createaccount
d57ad8f remove createaccount api
e13211c Merge pull request grpc-ecosystem#38 from tronprotocol/evan-rm-transaction-type
8aeba00 rm transaction type
c7495f8 Merge pull request grpc-ecosystem#36 from tronprotocol/add-reason-enum
59333a3 add reason
9aa4212 Merge pull request grpc-ecosystem#35 from tronprotocol/add_fullNode_API
e4025ff feat: delete GetTransactionByLimitPrev.
07d94e4 Merge pull request grpc-ecosystem#34 from tronprotocol/add_fullNode_API
884b666 fix: add http option.
5d93c0c feat: add fullnode GRPC.
08b0663 Merge commit 'c3b07944ee8eb47d7461fd304520364db41671ad' into develop
c3b0794 Merge pull request grpc-ecosystem#33 from tronprotocol/add_frozen_allowance
b7224ed add bandwidth in account
3464107 Merge branch 'develop' of https://github.com/tronprotocol/java-tron into develop
9313380 Merge pull request grpc-ecosystem#32 from tronprotocol/add_frozen_allowance
88da776 minor change
f12e2cc minor change
d18ae9c minor change
e18bdf2 add frozen and allowance balance
479564d mdf channel
f5e7632 Merge commit '647650de48d27934e0eb018db1c0ff4242ee39a5' into develop
647650d Merge pull request grpc-ecosystem#31 from tronprotocol/tx-timestamp
b290aad add hello message timestamp
6eba349 fix the transfer exception.
01a4166 Merge pull request grpc-ecosystem#30 from tronprotocol/update_account
80c5fd4 Merge branch 'master' into update_account
6cb5e8c Merge pull request grpc-ecosystem#528 from tronprotocol/refactor_transaction
7fc51a7 refactor the transaction structure
221cf70 Merge pull request grpc-ecosystem#525 from tronprotocol/refactor_transaction
7308a22 refactor the transaction structure
3868626 merge develop
a68e403 Merge commit 'd49019ec76c606ab1aa743a97710bac8b291cbf5' into add_grpc_api_for_solidity
d49019e Merge pull request grpc-ecosystem#28 from tronprotocol/fix_return
ff425d8 fix: return type.
1435bef Merge pull request grpc-ecosystem#27 from tronprotocol/add-reason-enum
2ec383f add ReasonCode
4d6bbda modify use LatestBlockHeaderNumber compare
d139597 modify use LatestBlockHeaderNumber compare
bc9ca33 merge protocol
8c6167e Merge pull request #25 from tronprotocol/xd-reason-code
130f5a2 modify reason code
6f41dfa Merge pull request #22 from tronprotocol/feature/add_db_api
9498448 feature: merge master to this.
13d4b62 Merge remote-tracking branch 'origin/master' into feature/add_db_api
246cf61 Merge pull request #24 from tronprotocol/add_database_api
2f7ae0b feature:add Datebase api Service.
a5e544c fix:change go_package.
c003d39 Merge remote-tracking branch 'origin/master' into feature/add_db_api
2dbee3b add WalletSolidity api service.
acc5c20 add get AssetIssueList by Timestamp api.
87c23fb add get Transaction api.
1033d54 Merge branch 'develop' into remove_header
c56402e add solidity node impl
f549760 add go package setting
7f1d31a add go package setting
0ef1de2 Merge branch 'develop' of https://github.com/tronprotocol/java-tron into develop
303dd5d feat: add BlockReference api
ed93332 Merge commit '1fab36b414599a3b24e739da4f04ee57bb4c14a5' into develop
d4a93ed improve: modified the seq num and fix build error
7acdfaf improve: add ref_block_num and ref_block_hash
c7f7d3e feat: remove utxo support from protocol.
59a3f02 Merge remote-tracking branch 'origin/develop' into develop
ac5eed9 add comments about ParticipateAssetIssueContract.
b1f5964 Merge commit 'ce4933fbe83c734ca244ecb0e8f8a9f854aa8afe' into protocol
336711d Merge commit '0297ff5d8a027576b0a7943cf0174ca7fc789534' into develop
f7d6d7b Merge commit 'ba4df0279d39dc07b6f6e40ccb0a66f3f5631e67' into feature/add_total_trans
0ed107a Merge commit '9f390c30504fe635e9d968393c2869f14cdfe9a4' into develop
4842483 Merge commit '06d60bdde8214ae47fecd8625e31dcd54c0bb324' into p2p
95b951b Merge branch 'develop' into p2p
192b1df Revert "Revert "Feature/devfor evan""
4e5ce06 Merge commit '9ac2d9b122b1e5ce429b4cc607741c56fbe27d4c' into p2p
fcc01fe Revert "Feature/devfor evan"
222e9e0 Merge remote-tracking branch 'origin/feature/devforEvan' into feature/devforEvan
0e407b9 merage proto
abb7a8b Merge commit '4668a8e92da61efdcb12d1bc7275d02373a7e052' into feature/devforEvan
e0dd5af Merge commit 'ca59ee900e2611553abebbf3a9f8feacab52228e' into feature/devforEvan
3f78d1b merge
94005a1 Merge commit 'ca59ee900e2611553abebbf3a9f8feacab52228e' into p2p
2836bdd change getBalance to getAccount
a97cfd7 Merge commit 'f1a1e24144dcefa5b604aa4a5ed69ef3bb934c66' into p2p
473587b merage proto
b74432d Merge remote-tracking branch 'origin/feature/devforEvan' into feature/devforEvan
312121f merage proto
faabdc4 Merge commit '044a39a4666b0428785d0501067bf417ade820c1' into p2p
82ceb8d Merge commit '13f1658d0497ecf4b6ce3ae9ed199b35cf654e21' into feature/devforEvan
f6ecbb5 Merge commit '0d9e0fdb313a8d4d5d3baaaea22569b72645fc08' into feature/devforEvan
8d0790f Merge commit 'bbeed13b2b052ae0fc669fb4a4ce1d5ac2cc9092' into p2p
8140964 Merge commit 'd08db0e07224a7aac3f296fc3a2e930e24d06fb5' into p2p
0aaff3d mdf discover msg
cf487bb Merge commit 'f63eda3db1b4386128cbc6235c3b95a72fbf0ba0' into p2p
dc9c1bb Merge commit '79c9b333f22367d3792f3faa2970b8dbbcf646b0' into feature/devforEvan
7abfb54 Merge commit '0f3ab59dfcba7e39789cdb810f3bd69a4fabb44b' into feature/devforEvan
5b0f8ac add getBlock
8901f2c add discover message
936b701 Merge commit '2f82ce89fcbebced73126885cf2feeda3463c73d' into develop
bbd0ea3 Merge commit '050740d1801ad990f2ad3c9eeee3030c74e022f3' into p2p
61decfb Merge commit '2795379f4fee8b12715e8835967f67925a5d68e7' into p2p
b5ced2d Merge branch 'develop' into junit/config
68b5a48 Merge pull request grpc-ecosystem#251 from sdargutev/develop
6b70dfd Merge pull request grpc-ecosystem#245 from tronprotocol/feature/updatewitness
935fd4c merged
c9410d8 refactoring
a3c7257 Merge remote-tracking branch 'origin/develop' into junit/config
9df3d33 Merge commit 'ec85aa1c2f62dbb6a027cd713e5965311dee29e6' into feature/updatewitness
fee98bf Merge remote-tracking branch 'origin/syncBlockChain' into syncBlockChain
7f4f302 rm duplicate instance
f509ce3 Merge branch 'develop' of https://github.com/tronprotocol/java-tron into syncBlockChain
774cceb Merge pull request grpc-ecosystem#237 from tronprotocol/miss_confuse_db
164d8bd remove dupicated number refresh
a58bf8e Merge branch 'develop' into junit/config
bb3a578 improve code readability
6910ef4 fix: add Account setter() validator

git-subtree-dir: protocol
git-subtree-split: e92f847
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