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

Add Pusher() function for support http2 server push #1273

Merged
merged 6 commits into from Jun 21, 2018

Conversation

htobenothing
Copy link
Contributor

gin already support http2, while previously not support server push.
Add Pusher() function to extend the ResponseWriter interface.

// get http.Pusher
 if pusher := c.Writer.Pusher(); pusher != nil {
     // use pusher.Push() to do server push
}

screen shot 2018-03-07 at 11 20 49 pm

@thinkerou
Copy link
Member

@htobenothing you should add some unit test case, and http.Pusher is supported only go1.8+, thanks!

@rvflash
Copy link

rvflash commented Mar 17, 2018

Hi @htobenothing,

I was going to make a pull request last night on the same subject.
I was thought to propose an interface like this, what do you think about it?

// ResponseWriter is used by an Gin HTTP handler to
// construct an HTTP response.
type ResponseWriter interface {
// ...
	// Pusher returns the http.Pusher for server push if it supported.
	Pusher() (http.Pusher, bool)
// ...
}

// Pusher returns the http.Pusher interface if supported by the response.
func (w *responseWriter) Pusher() (pusher http.Pusher, ok bool) {
    pusher, ok = w.ResponseWriter.(http.Pusher)
    return
}

@thinkerou How to do with the current official support of Go 1.6 and 1.7, without rewriting too much code ? A new minor version ?

@dreambo8563
Copy link

any progress on this feature?

@thinkerou
Copy link
Member

go1.6 and go1.7 will have the following error:

response_writer.go:42: undefined: http.Pusher

#1273 (comment)

@jsnathan
Copy link

Build tags could be used to conditionally compile the Pusher() method only on go >= 1.8

@codecov
Copy link

codecov bot commented Jun 20, 2018

Codecov Report

Merging #1273 into master will decrease coverage by 0.21%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1273      +/-   ##
==========================================
- Coverage   98.21%   97.99%   -0.22%     
==========================================
  Files          35       36       +1     
  Lines        1844     1848       +4     
==========================================
  Hits         1811     1811              
- Misses         26       30       +4     
  Partials        7        7
Impacted Files Coverage Δ
response_writer.go 100% <ø> (ø) ⬆️
response_writer_1.8.go 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87d536c...8ed5d8b. Read the comment docs.

@appleboy
Copy link
Member

@thinkerou @htobenothing @jsnathan @rvflash @dreambo8563

I updated the code base that supports go1.7 and go1.8 onward version now.

Please help to review and give me some feedbacks.

@thinkerou
Copy link
Member

Hi @appleboy in my option, ResponseWriter rename responseWriterBase(other name?), and then in response_writer_1.7.go we have the following code:

type ResponseWriter interface {
	responseWriterBase
}

and in response_writer_1.8.go we will see the following code:

type ResponseWriter interface {
	responseWriterBase

	// get the http.Pusher for server push
	Pusher() http.Pusher
}

func (w *responseWriter) Pusher() (pusher http.Pusher) {
	if pusher, ok := w.ResponseWriter.(http.ResponseWriter).(http.Pusher); ok {
		return pusher
	}
	return nil
}

prime target reduce duplicate code.

@appleboy
Copy link
Member

Add http2 push exmaple.

package main

import (
	"html/template"
	"log"

	"github.com/gin-gonic/gin"
)

var html = template.Must(template.New("https").Parse(`
<html>
<head>
  <title>Https Test</title>
  <script src="/assets/app.js"></script>
</head>
<body>
  <h1 style="color:red;">Welcome, Ginner!</h1>
</body>
</html>
`))

func main() {
	r := gin.Default()
	r.Static("/assets", "./assets")
	r.SetHTMLTemplate(html)

	r.GET("/", func(c *gin.Context) {
		if pusher := c.Writer.Pusher(); pusher != nil {
			// use pusher.Push() to do server push
			if err := pusher.Push("/assets/app.js", nil); err != nil {
				log.Printf("Failed to push: %v", err)
			}
		}
		c.HTML(200, "https", gin.H{
			"status": "success",
		})
	})

	// Listen and Server in https://127.0.0.1:8080
	r.RunTLS(":8080", "./testdata/server.pem", "./testdata/server.key")
}

@appleboy
Copy link
Member

@thinkerou thanks for your feedback. Done. see fa81624

@appleboy appleboy added this to the 1.3 milestone Jun 20, 2018
@thinkerou
Copy link
Member

LGTM, but should add some test case for Pusher.

@SCKelemen
Copy link

Thank you for adding this feature!

@appleboy appleboy merged commit bf85b32 into gin-gonic:master Jun 21, 2018
@rvflash
Copy link

rvflash commented Jun 22, 2018

Sorry for the late, I was in vacation. Thanks @appleboy @thinkerou @jsnathan for adding this feature!

@lggomez
Copy link
Contributor

lggomez commented Jun 25, 2018

We're having a regression in our code (go 1.8) due to a an updated gin version and I suspect this change is causing it

The following code

return func(c *gin.Context) {
    var wb *customWritter

    if w, ok := c.Writer.(gin.ResponseWriter); ok {
        wb = &customWritter{context: c, response: w, body: &bytes.Buffer{}}
        c.Writer = wb

yields this compilation error:

cannot use wb (type *customWritter) as type gin.ResponseWriter in assignment:
       *customWritter does not implement gin.ResponseWriter (missing Pusher method)

Is there a workaround for this? otherwise we should stick strictly to v1.2.0

@thinkerou
Copy link
Member

@lggomez you should use the master branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants