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

Jump to bottom

net/http: document explicitly that http2 doesn't support Hijacker #15312

Closed
clementino opened this issue Apr 15, 2016 · 4 comments
Closed

net/http: document explicitly that http2 doesn't support Hijacker #15312

clementino opened this issue Apr 15, 2016 · 4 comments
Labels
Documentation FrozenDueToAge
Milestone
Go1.7

Comments

@clementino
Copy link

clementino commented Apr 15, 2016 •
edited

When I upgraded to go1.6 (Linux amd64) our applications, which rely heavily on Server-sent events, stopped working because *http2responseWriter does not implement http.Hijacker (#14797) (tried Firefox only).

I'm not using any SSE package, see code below (slightly edited), and it worked fine for 4 years until go1.6.

As I said in the referenced issue, I think there is a problem with the documentation because I relied on a feature that stopped working without warning.
I propose to document that *http2responseWriter is not an Hijacker and any other possible case, so that people can evaluate if and when to rely on the hijacking feature and how to set up the server so that it may fail only for external causes.
It may seems that there are no sensible uses for hijacking besides websockets, but you never know, in fact I was using it for SSE.

EDIT: I realize I can't expect my code to work across http versions if I hardcode the version myself, but even if the version was correct it doesn't reach past the type assertion. How should I go about rewriting this without hijacking?

var sseHeader = "HTTP/1.1 200 OK\nContent-Type: text/event-stream\n\n"

func handleSse(w http.ResponseWriter, r *http.Request) {
    conn, bufrw, err := w.(http.Hijacker).Hijack()
    if err != nil {
        http.Error(w, "hijack failed", http.StatusInternalServerError)
        return
    }
    if _, err := bufrw.WriteString(sseHeader); err != nil {
        conn.Close()
        return
    }
    if err := bufrw.Flush(); err != nil {
        conn.Close()
        return
    }
    newSse(func(msg []byte) (ok bool) {
        for _, line := range bytes.Split(msg, []byte("\n")) {
            if _, err := bufrw.WriteString("data: "); err != nil {
                conn.Close()
                return
            }
            if _, err := bufrw.Write(line); err != nil {
                conn.Close()
                return
            }
            if err := bufrw.WriteByte('\n'); err != nil {
                conn.Close()
                return
            }
        }
        if err := bufrw.WriteByte('\n'); err != nil {
            conn.Close()
            return
        }
        if err := bufrw.Flush(); err != nil {
            conn.Close()
            return
        }
        return true
    })
}
@bradfitz
Copy link
Contributor

The docs on https://golang.org/pkg/net/http/#Hijacker do already say "The Hijacker interface is implemented by ResponseWriters that allow an HTTP handler to take over the connection", which is trying to say that not all ResponseWriters support that. If they did, it would just be part of the ResponseWriter interface.

To do this without hijack, just write to your ResponseWriter and use https://golang.org/pkg/net/http/#Flusher instead. Both http1 and http2 support Flusher (but you should still test for it and conditionally use it). There's no need to use a buffered writer. The http1 and http2 response writers are already buffered. (and any unbuffered one wouldn't need a flush)

@bradfitz
Copy link
Contributor

I'll repurpose this bug to be about documentation.

@bradfitz bradfitz reopened this Apr 15, 2016
@bradfitz bradfitz self-assigned this Apr 15, 2016
@bradfitz bradfitz changed the title Server-sent events implemented using http hijacking is broken by http2 without warning net/http: document explicitly that http2 doesn't support Hijacker Apr 15, 2016
@bradfitz bradfitz added this to the Go1.7 milestone Apr 15, 2016
@clementino
Copy link
Author

Thanks, your solution works fine!
I thought that besides flushing, without hijacking I wouldn't be able to disable chunking in the ResponseWriter and that SSE wouldn't work with chunking enabled.
But I was wrong in both cases: chunking is not recommended for SSE but allowed, and to disable it all I had to do was to change the Transfer-Encoding header.
Sorry.

About the docs I understand. I'm not saying that it is incorrect, I'm saying that it could be more useful.
I don't choose a ResponseWriter over another, I just use the one that is given to me. If a feature depends on some specific ResponseWriter and I depend on such a feature I want to have an idea when it may not be available.

I understand that use cases for Hijacker are limited but I'm glad you decided to document it, thanks.

@gopherbot
Copy link

CL https://golang.org/cl/22122 mentions this issue.

@golang golang locked and limited conversation to collaborators Apr 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge
Projects
None yet
Milestone
Go1.7
Development

No branches or pull requests
3 participants
@bradfitz @clementino @gopherbot