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

proposal: net/http: flush other headers with 100-continue in expectContinueReader #40838

Open
acud opened this issue Aug 17, 2020 · 2 comments
Open
Labels
Projects
Milestone

Comments

@acud
Copy link

@acud acud commented Aug 17, 2020

On our http server that uses the built-in go http server, we'd like to pass a custom header to the API consumer before the final HTTP response has been established. It is documented that the golang http server does not support sending custom 1xx status codes, however the golang http server itself sends a 100-Continue once the request reader is being read with the expectCustomReader.

So the proposal is as follows: respect what is being written to the response writer before the 100-Continue is being written into the response, then flush it together with the 100-Continue which is sent anyway.

I believe many users can benefit from this, would be happy to hear what you think (and I can also own this and PR it into the codebase).

Thanks a million.

@gopherbot gopherbot added this to the Proposal milestone Aug 17, 2020
@gopherbot gopherbot added the Proposal label Aug 17, 2020
@ianlancetaylor ianlancetaylor changed the title proposal: flush other headers with 100-continue in expectContinueReader proposal: net/http: flush other headers with 100-continue in expectContinueReader Aug 17, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Aug 17, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 17, 2020

I don't know this code very well but I don't understand how that could work reliably. expectContinueReader is based on reading the request, so I don't see how the requester responder will be able to add any headers.

@acud
Copy link
Author

@acud acud commented Aug 20, 2020

well, without having plotted out a detailed implementation plan it seems to me that the main thing here is that responsibility for writing the headers is leaked to the reader in the case of expectContinueReader, which is not ideal. The reader is invoking the write on the actual connection upon read:

func (ecr *expectContinueReader) Read(p []byte) (n int, err error) {
	if ecr.closed {
		return 0, ErrBodyReadAfterClose
	}
	w := ecr.resp
	if !w.wroteContinue && w.canWriteContinue.isSet() && !w.conn.hijacked() {
		w.wroteContinue = true
		w.writeContinueMu.Lock()
		if w.canWriteContinue.isSet() {
			w.conn.bufw.WriteString("HTTP/1.1 100 Continue\r\n\r\n") <--- this
			w.conn.bufw.Flush()
			w.canWriteContinue.setFalse()
		}
		w.writeContinueMu.Unlock()
	}
...

So essentially, if we can proxy the write through the actual response object in a dedicated method we could respect the headers that were written into it in the meanwhile. The only thing is that infers that synchronization is needed in order to make sure that no new headers are being written while the previous ones are being flushed to the client. We could get around this in order to get better performance if synchronization is the deal-breaker here, but let's first discuss if this is a viable solution at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

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