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

proposal: net/http: add Request.SetContext #48811

Closed
lesismal opened this issue Oct 6, 2021 · 8 comments
Closed

proposal: net/http: add Request.SetContext #48811

lesismal opened this issue Oct 6, 2021 · 8 comments
Labels
FrozenDueToAge Proposal WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@lesismal
Copy link

lesismal commented Oct 6, 2021

related: #48339

@gopherbot gopherbot added this to the Proposal milestone Oct 6, 2021
@neild
Copy link
Contributor

neild commented Oct 6, 2021

Copying a comment from CL 349369:

The original change which introduced a Context to Request deliberately made this field unexported and provided no way to set the context for a Request without copying it. A proposal to change this should explain what has changed since then. See in particular rsc@'s comment here:
#14660 (comment)

@seankhliao seankhliao added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 6, 2021
@lesismal
Copy link
Author

lesismal commented Oct 6, 2021

For the std, the prohibition for context field is reasonable.

But for non-std frameworks that want to be compatible with std Handler, this limits the implementation of non-std frameworks. Although I've found a way here, it wastes more.

The std solution——one or more goroutines per connection, when handling a 1000k scenario, cost huge memory, scheduling, GC, and leads to STW more easily and it even performs obviously worse than java-netty and nodejs. So there are still people of us in the community who try to use the syscall+ poller to solve the 1000k problem, we save a lot of hardware configuration and bring better stability.

@neild
Copy link
Contributor

neild commented Oct 6, 2021

I'm sorry, but I don't see what Request.SetContext has to do with numbers of goroutines.

@lesismal
Copy link
Author

lesismal commented Oct 6, 2021

I'm sorry, but I don't see what Request.SetContext has to do with numbers of goroutines.

Sorry for that, I forgot to explain.

Here I want to set the context field, but there's no Request.SetContext method, so I set the whole Request struct with another struct that has the context value.

@lesismal
Copy link
Author

lesismal commented Oct 6, 2021

nbio is a poller framework, it saves goroutines and aims to solve 1000k, also wants to be compatible with std Handler as much as possible, so I want to set the Request.context

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Oct 6, 2021
@rsc
Copy link
Contributor

rsc commented Oct 6, 2021

Request is explicitly immutable. That's why we have Request.WithContext.

@rsc
Copy link
Contributor

rsc commented Oct 6, 2021

Declining as infeasible (contradicts basic design).

@rsc rsc moved this from Incoming to Declined in Proposals (old) Oct 6, 2021
@rsc
Copy link
Contributor

rsc commented Oct 6, 2021

This proposal has been declined as infeasible.
— rsc for the proposal review group

@rsc rsc closed this as completed Oct 6, 2021
@golang golang locked and limited conversation to collaborators Oct 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
No open projects
Development

No branches or pull requests

5 participants