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

Support streaming #435

Closed
hexfusion opened this issue Jul 16, 2017 · 3 comments
Closed

Support streaming #435

hexfusion opened this issue Jul 16, 2017 · 3 comments

Comments

@hexfusion
Copy link
Contributor

hexfusion commented Jul 16, 2017

In certain HTTP 1.1 streaming circumstances the runtime handler calls Flush when it should instead fallback to internal net/httpd handling of "flush after chunk" outlined in this commit.
golang/go@e5febf9 which references issue golang/go#6574

Example of how to replicate issue via newline delimited JSON request. etcd-io/etcd#8237

To allow this flexibility I propose a header ie Grpc-Metadata-AutoFlush (name?) which will conditionally disable manual Flush, thus falling back to the internal net/http handling. The opt-in nature of this should be unobtrusive yet allow this type of streaming request to properly return. Also by using the Grpc-Metadata- naming it will provide useful access to the metadata key.

I was thinking something like below in runtime/handler.go

// replaces  f.Flush()
checkFlush(req,f)

// unless header is found manually Flush
func checkFlush(req *http.Request, f http.Flusher) {
    if af := req.Header.Get("Grpc-Metadata-AutoFlush"); af == "" {
        f.Flush()
    }
    return
}

Thank you for your consideration and input.
PR ref: #436

@achew22
Copy link
Collaborator

achew22 commented Jul 24, 2017

Right now grpc-gateway doesn't support streaming over HTTP/1.1 the way you propose. @tmc and I have talked about adding the ability to do streaming to grpc-gateway, but we haven't yet landed on how to actually do it while being faithful to the spec. We will definitely take this effort into consideration when we work out streaming support, but for now this is a low priority item for me. If this is something you'd be interested in taking on (or any other open issue/FR) I would love to help point you to the right place. I'm going to close this to keep our backlog clean-ish but feel free to reopen it/turn it into a PR if you would like to take a swing at it.

@achew22 achew22 closed this as completed Jul 24, 2017
@hexfusion
Copy link
Contributor Author

@achew22 thank you for the notes and kind invitation to provide recon. I hope to revisit this and will make sure to touch base with you before hand to compare notes.

@hexfusion
Copy link
Contributor Author

hexfusion commented Aug 15, 2017

@achew22 I think this functionality would really benefit the project and would like to take this on if for nothing else to better understand how grpc-gateway works. If you have any notes you could pass my way or tickets, in particular, outlining your thoughts/requirements, they would be appreciated. Thanks!

@hexfusion hexfusion changed the title Support disabling manual Flush via header. Support streaming Aug 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants