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

rpc: support inject http headers using outgoing context #26023

Merged
merged 14 commits into from
Nov 16, 2022

Conversation

storyicon
Copy link
Contributor

this PR is used to solve: #26004

Signed-off-by: storyicon yuanchao@bilibili.com

Signed-off-by: storyicon <yuanchao@bilibili.com>
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

I think the idea is fine. Curious what @fjl thinks...?

rpc/metadata.go Outdated Show resolved Hide resolved
rpc/metadata.go Outdated Show resolved Hide resolved
rpc/metadata.go Outdated Show resolved Hide resolved
rpc/metadata.go Outdated Show resolved Hide resolved
rpc/http_test.go Outdated Show resolved Hide resolved
storyicon and others added 4 commits October 23, 2022 10:17
Co-authored-by: Martin Holst Swende <martin@swende.se>
Co-authored-by: Martin Holst Swende <martin@swende.se>
Signed-off-by: storyicon <yuanchao@bilibili.com>
Signed-off-by: storyicon <yuanchao@bilibili.com>
@storyicon
Copy link
Contributor Author

storyicon commented Oct 23, 2022

@holiman thanks, I adopted some of your suggestions and simplified the design. Now it has become clearer about the requirement of http.Header injection.

rpc/metadata.go Outdated Show resolved Hide resolved
rpc/metadata.go Outdated Show resolved Hide resolved
Signed-off-by: storyicon <yuanchao@bilibili.com>
@holiman
Copy link
Contributor

holiman commented Oct 25, 2022

I pushed a change now, mostly small things, but one functional change -- it now supports multiple contexts setting headers sequentially. Please take a look.

I think this is ok now, leaving it to @fjl to decide about

Signed-off-by: storyicon <yuanchao@bilibili.com>
@storyicon
Copy link
Contributor Author

I have also made some changes to make the logic clearer. At the same time, NewContextWithHeaders will not modify the http.Header passed in by the user. Now it should be OK.

rpc/context_headers.go Outdated Show resolved Hide resolved
rpc/http_test.go Show resolved Hide resolved
storyicon and others added 2 commits October 29, 2022 17:42
Co-authored-by: Martin Holst Swende <martin@swende.se>
Co-authored-by: Martin Holst Swende <martin@swende.se>
@storyicon
Copy link
Contributor Author

@holiman What else can we do to promote this? It seems that @fjl missed some message.

@holiman
Copy link
Contributor

holiman commented Nov 8, 2022 via email

@storyicon
Copy link
Contributor Author

Nah, don't worry, he'll get to it eventually. We are slow with PRs often, let it take it's time

Thank you for actively addressing this issue.

@fjl fjl added this to the 1.11.0 milestone Nov 16, 2022
@fjl fjl merged commit add337e into ethereum:master Nov 16, 2022
shekhirin pushed a commit to shekhirin/go-ethereum that referenced this pull request Jun 6, 2023
This adds a way to specify HTTP headers per request.

Co-authored-by: Martin Holst Swende <martin@swende.se>
Co-authored-by: Felix Lange <fjl@twurst.com>
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.

4 participants