Skip to content

net/http: avoid casting and extra function calling (http.HandleFunc, http.DefaultServeMux.HandleFunc) #25706

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

Closed
ntrrg opened this issue Jun 2, 2018 · 14 comments
Labels
FrozenDueToAge Proposal v2 An incompatible library change
Milestone

Comments

@ntrrg
Copy link

ntrrg commented Jun 2, 2018

Looking at the net/http package I notice that HandleFunc and ServeMux.HandleFunc have a func(ResponseWriter, *Request) as parameter instead of HandlerFunc, this means that this functions need to call one extra fuction and an explicit casting respectively.

So if they are written in this way, it can be avoided:

HandleFunc

Since HandlerFunc implements the Handler interface, DefaultServeMux.HandleFunc should be replaced by DefaultServeMux.Handle.

From:

func HandleFunc(pattern string, handler func(ResponseWriter, *Request)) {
  DefaultServeMux.HandleFunc(pattern, handler)
}

To:

func HandleFunc(pattern string, handler HandlerFunc) {
  DefaultServeMux.Handle(pattern, handler)
}

ServeMux.HandleFunc

Since the handler argument is implicitly casted (or something else happens here?), an explicit casting is not necessary.

From:

func (mux *ServeMux) HandleFunc(pattern string, handler func(ResponseWriter, *Request)) {
  if handler == nil {
    panic("http: nil handler")
  }
  mux.Handle(pattern, HandlerFunc(handler))
}

To:

func (mux *ServeMux) HandleFunc(pattern string, handler HandlerFunc) {
  if handler == nil {
    panic("http: nil handler")
  }

  mux.Handle(pattern, handler)
}
@meirf
Copy link
Contributor

meirf commented Jun 3, 2018

@ntrrg The golang/go issues are meant for bugs or feature requests - not general questions.

I recommend closing this issue and taking the following steps until you get a satisfactory answer to your question:

  1. Read this explanation of DefaultServeMux: https://stackoverflow.com/a/36921591/1168364
  2. Ask a question in https://groups.google.com/forum/#!forum/golang-nuts or https://stackoverflow.com/questions/ask
  3. If the discussion in 2 shows there's a bug in the standard library or that there is some standard behavior missing I recommend opening an issue.

@ntrrg
Copy link
Author

ntrrg commented Jun 3, 2018

Great, thanks! 😄 I was a little confused because this is actually more a proposal than a question.

@ntrrg ntrrg closed this as completed Jun 3, 2018
@meirf
Copy link
Contributor

meirf commented Jun 3, 2018

This is definitely the right place to create a proposal, but to be honest your proposal is not very clear to me. I leaned toward assuming this was just a question because of the word "wonder".

If you'd like to try again to explain, please reply with more detail on what exactly you're proposing.

@ntrrg
Copy link
Author

ntrrg commented Jun 3, 2018

Your are right 😆 I will try to make it clearer, so please, read the description again. I am still learning English btw 😅

@ntrrg ntrrg reopened this Jun 3, 2018
@ntrrg ntrrg changed the title net/http: avoid casting (http.HandleFunc, http.DefaultServeMux.HandleFunc) net/http: avoid casting and extra function calling (http.HandleFunc, http.DefaultServeMux.HandleFunc) Jun 3, 2018
@davecheney
Copy link
Contributor

davecheney commented Jun 3, 2018

Please send a change when the Go 1.12 cycle opens in a few months. Thank you.

https://golang.org/doc/contribute.html

@ntrrg
Copy link
Author

ntrrg commented Jun 3, 2018

Great! 😄 I suppose that this issue should keep open, right?

@meirf
Copy link
Contributor

meirf commented Jun 3, 2018

Yes, please keep open.

@ghost
Copy link

ghost commented Jun 3, 2018

As it stands now, the proposed change cannot happen in Go 1, as it is changing the signature of public methods.

The only way this might be able to work is if HandlerFunc was also changed to a type alias of func(ResponseWriter, *Request), i.e.:

type HandlerFunc = func(ResponseWriter, *Request)

Someone who knows the code better would be able to explain any problems in making such a change.

Type alias makes no sense, as ServeHTTP needs to be defined on HandlerFunc.

@ntrrg
Copy link
Author

ntrrg commented Jun 3, 2018

@bontibon Actually it already exists here, that is the reason of this proposal 😄

@FiloSottile FiloSottile added this to the Proposal milestone Jun 5, 2018
@FiloSottile FiloSottile added Proposal v2 An incompatible library change labels Jun 5, 2018
@ntrrg
Copy link
Author

ntrrg commented Jun 7, 2018

@FiloSottile I think that this could be added in the next release because doesn't break any code and is invisible to any user. But this needs a modification in api/go1.txt, this file can't be modified? Even if the modification doesn't break the API?

@agnivade
Copy link
Contributor

agnivade commented Jun 7, 2018

@ntrrg
Copy link
Author

ntrrg commented Jun 7, 2018

Yep, sorry, the next next release (1.12) i mean 😆

@ianlancetaylor
Copy link
Contributor

This is an API change that is forbidden by the Go 1 guarantee. It would break code that said

var x func(string, func(http.ResponseWriter, *http.Request)) = http.HandleFunc

So this issue is correctly labeled as a Go 2 issue.

Closing this issue in favor of #5465, where this change could be made as part of a future net/http/v2.

@ntrrg
Copy link
Author

ntrrg commented Sep 28, 2018

Yes, you are right, sorry about that 😅

@golang golang locked and limited conversation to collaborators Sep 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal v2 An incompatible library change
Projects
None yet
Development

No branches or pull requests

7 participants