Make middleware mountable on http.NewMuxer()

[tchssk]

I added SmartRedirectSlashes middleware in #3394 based on #3385's discussion, but it couldn't be used with default NewMuxer(). The middleware need to be mounted at the top level of chi's router, but a router created by NewMuxer() already mount the Not Found handler.
Solve this problem by adding an argument to NewMuxer().

[raphael]

Thank you for the PR, that makes sense. I wonder if as an alternative we couldn't store the middlewares defined via Use and register them together with the NotFound route on first call to Handle. Something like:

// mux is the default Muxer implementation.
mux struct {
    chi.Router
    // protect access to middlewares and wildcards
    lock sync.Mutex
    // used to register middlewares during first Handle call
    once sync.Once
    // middlewares to be registered before handlers
    middlewares []func(http.Handler) http.Handler
    // wildcards maps a method and a pattern to the name of the wildcard
    // this is needed because chi does not expose the name of the wildcard
    wildcards map[string]string
}

// ...

// NewMuxer returns a Muxer implementation based on a Chi router.
func NewMuxer() ResolverMuxer {
	r := chi.NewRouter()
	return &mux{Router: r, wildcards: make(map[string]string), middlewares: []func(http.Handler) http.Handler{}}
}

// ...
func (m *mux) Handle(method, pattern string, handler http.HandlerFunc) {
    m.once.Do(func() {
        for _, mw := range m.middlwares {
            m.Router.Use(m)
        }
	m.Router.NotFound(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
		ctx := context.WithValue(req.Context(), AcceptTypeKey, req.Header.Get("Accept"))
		enc := ResponseEncoder(ctx, w)
		w.WriteHeader(http.StatusNotFound)
		enc.Encode(NewErrorResponse(ctx, fmt.Errorf("404 page not found"))) // nolint:errcheck
	}))
        m.middlewares = nil
    })
    // ...
}

func (m *mux) Use(f func(http.Handler) http.Handler) {
     if m.middlewares == nil {
        m.Router.Use(f)
        return
    }
    m.middlewares = append(m.middlewares, f)
}

(note: I didn't test the code above - this is just to explain the idea)
This is a little bit more complicated but the benefit is that we don't have to change any signature/contract which helps if we want to change the router in the future. Also while looking at the above I realized that there is a bug currently where access to the wildcards map isn't protected by a mutex. This means that there could be a panic in the case multiple goroutines access the map concurrently (which is possible given it is used in methods that could be called from HTTP requests).

[tchssk]

The idea looks good to me. I'll try to implement it.

Also while looking at the above I realized that there is a bug currently where access to the wildcards map isn't protected by a mutex. This means that there could be a panic in the case multiple goroutines access the map concurrently (which is possible given it is used in methods that could be called from HTTP requests).

They are all read access, right? An error occurs with no write access? (My misunderstanding?)

Edit: Did you mean mux.Handle() is not goroutine safe? It seems like it can be improved by using sync.Map for mux.wildcards, but it may be too much.

Implemented a sync.Map version #3417

[raphael]

The idea looks good to me. I'll try to implement it.

That's great.

They are all read access, right? An error occurs with no write access? (My misunderstanding?)

No misunderstanding, you are correct that concurrent reads are safe.

Edit: Did you mean mux.Handle() is not goroutine safe? It seems like it can be improved by using sync.Map for mux.wildcards, but it may be too much.

Implemented a sync.Map version #3417

That's perfect, the edge case is indeed if Handle is called in a request handler which should be rare.

[tchssk]

Resolved by #3419