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

net/http: TimeoutHandler does not support Pusher interface #29193

Closed
inliquid opened this issue Dec 12, 2018 · 12 comments
Closed

net/http: TimeoutHandler does not support Pusher interface #29193

inliquid opened this issue Dec 12, 2018 · 12 comments

Comments

@inliquid
Copy link

@inliquid inliquid commented Dec 12, 2018

When used as a middleware http.TimeoutHandler breaks HTTP/2.0 Server Push mechanism, as its own writer does not support http.Pusher interface.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Dec 13, 2018

@inliquid can you give an example code?

@inliquid

This comment has been minimized.

Copy link
Author

@inliquid inliquid commented Dec 13, 2018

  1. Set up middleware
func setupHandler(r *mux.Router) {
	if cfg.ServerUseGzip {
		server.Handler = alice.New(
			loggerMiddleware,
			throttleMiddleware,
			//timeoutMiddleware,  // <-- breaks Push
			compressMiddleWare,
			recoverMiddleware,
			sessionMiddleware,
			authMiddleware,
		).Then(r)
	} else {
		server.Handler = alice.New(
			loggerMiddleware,
			throttleMiddleware,
			//timeoutMiddleware,   // <-- breaks Push
			recoverMiddleware,
			sessionMiddleware,
			authMiddleware,
		).Then(r)
	}
}
  1. timeoutMiddleware returns http.TimeoutHandler and is quite simple
func timeoutMiddleware(next http.Handler) http.Handler {
	return http.TimeoutHandler(next, time.Duration(cfg.ServerHandlerTimeout)*time.Second, http.StatusText(http.StatusServiceUnavailable))
}
  1. Push something
	p, ok := w.(http.Pusher)
	if ok {
		if err := p.Push("/assets/css/pure/pure.css", &http.PushOptions{
			//Method: "GET",
			Header: http.Header{
				"Accept-Encoding": []string{"gzip"},
			},
		}); err != nil {
			logger.Error.Println("Error pushing pure.css!")
		}
	}

When timeoutMiddleware uncommented ok == false and p == nil

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 17, 2018

Change https://golang.org/cl/154383 mentions this issue: net/http: TimeoutHandler supports server Push

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Dec 17, 2018

@inliquid Ah sorry, the example can be more simple by using code from blog. I mis-interpreted the response when trying it.

Can you try above commit if it's fixed for you.

@inliquid

This comment has been minimized.

Copy link
Author

@inliquid inliquid commented Dec 17, 2018

Ok, is there any instruction how to patch and test stdlib with these changes applied?

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Dec 18, 2018

@inliquid I think there's no way unless you re-build your go binary from source.

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Feb 18, 2019

Thank you for filing this bug @inliquid and @Gnouc for the preliminary questions.

@Gnouc in regards to your question in #29193 (comment)
here is a minimal repro to demonstrate this problem https://play.golang.org/p/e6Pr0U_pMCp or inlined below

package main

import (
	"log"
	"net/http"
	"time"
)

func main() {
	hf := http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
		rw.Write([]byte("OK"))
	})
	handler := http.TimeoutHandler(hf, 55*time.Second, "Timed out!")
	_, ok := handler.(http.Pusher)
	if !ok {
		log.Fatal("TimeoutHandler does not implement http.Pusher")
	}
	log.Print("TimeoutHandler implements http.Pusher")
}

which will print out

TimeoutHandler does not implement http.Pusher

Thanks @Gnouc for the CL, we shall review it for Go1.13.

@bradfitz might you have a wishlist for interfaces that many of these helpers should implement? If so, let's review collaborate on this.

@odeke-em odeke-em self-assigned this Feb 18, 2019
@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Feb 26, 2019

@odeke-em In case we have the wishlist, shall we implement in this issue or raise another one?

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Feb 26, 2019

@Gnouc great question! So I think we should spin off another issue after we get the wishlist. That way if any rollback needs to be performed, it'll be piecemeal and this CL will stay in solid :)

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Feb 27, 2019

@bradfitz can you please take a look?

@gopherbot gopherbot closed this in 2889332 Mar 2, 2019
jmhodges added a commit to jmhodges/go that referenced this issue Aug 22, 2019
As of Go 1.13rc1, TimeoutHandler supports the Flusher and Pusher interfaces and
this change corrects its documentation to say that.

Fixes golang#33769
Updates golang#29193
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 22, 2019

Change https://golang.org/cl/191237 mentions this issue: net/http: change TimeoutHandler's docs to match its new interfaces

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 22, 2019

Change https://golang.org/cl/191169 mentions this issue: [release-branch.go1.13] net/http: change TimeoutHandler's docs to match its new interfaces

gopherbot pushed a commit that referenced this issue Aug 22, 2019
As of Go 1.13rc1, TimeoutHandler supports the Flusher and Pusher interfaces and
this change corrects its documentation to say that.

Fixes #33769
Updates #29193

Change-Id: Ia0523f7f2e3dc1f8f0b68950b85a7bf81c4abe60
GitHub-Last-Rev: 5310d2c
GitHub-Pull-Request: #33770
Reviewed-on: https://go-review.googlesource.com/c/go/+/191237
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Run-TryBot: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Aug 22, 2019
…ch its new interfaces

As of Go 1.13rc1, TimeoutHandler supports the Flusher and Pusher interfaces and
this change corrects its documentation to say that.

Fixes #33769
Updates #29193

Change-Id: Ia0523f7f2e3dc1f8f0b68950b85a7bf81c4abe60
GitHub-Last-Rev: 5310d2c
GitHub-Pull-Request: #33770
Reviewed-on: https://go-review.googlesource.com/c/go/+/191237
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Run-TryBot: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit f3e3b71)
Reviewed-on: https://go-review.googlesource.com/c/go/+/191169
Reviewed-by: Bryan C. Mills <bcmills@google.com>
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
As of Go 1.13rc1, TimeoutHandler supports the Flusher and Pusher interfaces and
this change corrects its documentation to say that.

Fixes golang#33769
Updates golang#29193

Change-Id: Ia0523f7f2e3dc1f8f0b68950b85a7bf81c4abe60
GitHub-Last-Rev: 5310d2c
GitHub-Pull-Request: golang#33770
Reviewed-on: https://go-review.googlesource.com/c/go/+/191237
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Run-TryBot: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.