[go] net/http: add context cancellation reason for server handlers
Gopher Robot (Gerrit)
Gopher Robot added 1 comment![]( "Open in Gerrit")
I spotted some possible problems.
These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. Otherwise, please address any problems and update the GitHub PR. When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.
Possible problems detected:
1. It looks like you have a properly formated bug reference, but the convention is to put bug references at the bottom of the commit message, even if a bug is also mentioned in the body of the message.
The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
net/http: add context cancellation reason for server handlers
When we cancel a HTTP server handler context, set an appropriate
cancel cause. This makes investigation of context cancellation
errors easier.
Fixes #64465
To do:
- I've added two new errors to `net/http`. In order to make the tests succeed, I haven't exposed them, but I think they should be exposed to use with `errors.Is()`. The relevant proposal is at: https://github.com/golang/go/issues/64465#issuecomment-2543377045
Change diff
diff --git a/src/net/http/server.go b/src/net/http/server.go
index 1e8e143..5a5aecb 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -59,6 +59,17 @@
// anything in the net/http package. Callers should not
// compare errors against this variable.
ErrWriteAfterFlush = errors.New("unused")
+
+ // TODO: expose once proposal is accepted
+ // errConnectionClosed is used as a context Cause for contexts
+ // cancelled because the client closed their connection while
+ // a request was being handled.
+ errConnectionClosed = errors.New("connection closed")
+
+ // TODO: expose once proposal is accepted
+ // errConnectionHandled is used as a context Cause for contexts
+ // cancelled because the request handler returned.
+ errConnectionHandled = errors.New("connection handled")
)
// A Handler responds to an HTTP request.
@@ -257,7 +268,7 @@
server *Server
// cancelCtx cancels the connection-level context.
- cancelCtx context.CancelFunc
+ cancelCtx context.CancelCauseFunc
// rwc is the underlying network connection.
// This is never wrapped by other types and is the value given out
@@ -754,8 +765,11 @@
// down its context.
//
// It may be called from multiple goroutines.
-func (cr *connReader) handleReadError(_ error) {
- cr.conn.cancelCtx()
+func (cr *connReader) handleReadError(err error) {
+ if errors.Is(err, io.EOF) {
+ err = errConnectionClosed
+ }
+ cr.conn.cancelCtx(err)
cr.closeNotify()
}
@@ -2005,9 +2019,9 @@
// HTTP/1.x from here on.
- ctx, cancelCtx := context.WithCancel(ctx)
+ ctx, cancelCtx := context.WithCancelCause(ctx)
c.cancelCtx = cancelCtx
- defer cancelCtx()
+ defer cancelCtx(errConnectionHandled)
c.r = &connReader{conn: c}
c.bufr = newBufioReader(c.r)
@@ -4021,7 +4035,7 @@
n, err = w.c.rwc.Write(p)
if err != nil && w.c.werr == nil {
w.c.werr = err
- w.c.cancelCtx()
+ w.c.cancelCtx(err)
}
return
}
Change information
- M src/net/http/server.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Message from Gopher Robot![]( "Open in Gerrit")
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 1 comment![]( "Open in Gerrit")
I spotted some possible problems.
These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. Otherwise, please address any problems and update the GitHub PR. When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.
Possible problems detected:
1. It looks like you have a properly formated bug reference, but the convention is to put bug references at the bottom of the commit message, even if a bug is also mentioned in the body of the message.The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
The TODO will be removed as a result of review of the proposal.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 1 comment![]( "Open in Gerrit")
With this PR, the following code:
```
package main
import (
"context"
"log"
"time"
"net/http"
)
func main() {
http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) {
ctx := req.Context()
time.Sleep(2 * time.Second)
log.Printf("Context error: %+v", ctx.Err())
log.Printf("Context cause: %+v", context.Cause(ctx))
}) log.Printf("Listening on port 8090. To send a request:")
log.Printf(" curl http://localhost:8090/")
log.Printf("Then, cancel (^C) within 2 seconds to see the cancellation reason.")
http.ListenAndServe(":8090", nil)
}
```Provides the following output when run accordingly:
```
$ timeout 5 ~/Projecten/go/go/bin/go run . & timeout 2 bash -c 'sleep 1 && curl http://localhost:8090/' & wait
[1] 71823
[2] 71824
2024/12/15 01:08:05 Listening on port 8090. To send a request:
2024/12/15 01:08:05 curl http://localhost:8090/
2024/12/15 01:08:05 Then, cancel (^C) within 2 seconds to see the cancellation reason.
[2] + 71824 exit 124 timeout 2 bash -c 'sleep 1 && curl http://localhost:8090/'
2024/12/15 01:08:07 Context error: context canceled
2024/12/15 01:08:07 Context cause: connection closed
[1] + 71823 exit 124 timeout 5 ~/Projecten/go/go/bin/go run .
```
Compare the output line "context cause" with the behaviour as of Go 1.23:
```
$ timeout 5 go run . & timeout 2 bash -c 'sleep 1 && curl http://localhost:8090/' & wait
[1] 68374
[2] 68375
2024/12/15 01:06:27 Listening on port 8090. To send a request:
2024/12/15 01:06:27 curl http://localhost:8090/
2024/12/15 01:06:27 Then, cancel (^C) within 2 seconds to see the cancellation reason.
[2] + 68375 exit 124 timeout 2 bash -c 'sleep 1 && curl http://localhost:8090/'
2024/12/15 01:06:29 Context error: context canceled
2024/12/15 01:06:29 Context cause: context canceled
[1] + 68374 exit 124 timeout 5 go run .
```
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Himanshu Pathak (Gerrit)
Himanshu Pathak added 2 comments![]( "Open in Gerrit")
Hi! Thanks for working on this improvement. I often find it hard to distinguish the error when the server is acting as a rest api and the client is rapidly flipping through pages triggering requests and cancelling before the request finishes.
errConnectionClosed = errors.New("connection closed")I would like to see a more descriptive message in this error `connection closed by client` or similar. Thank you
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 2 comments![]( "Open in Gerrit")
Hi! Thanks for working on this improvement. I often find it hard to distinguish the error when the server is acting as a rest api and the client is rapidly flipping through pages triggering requests and cancelling before the request finishes.
Thank you!
errConnectionClosed = errors.New("connection closed")I would like to see a more descriptive message in this error `connection closed by client` or similar. Thank you
- Damien Neil
- Himanshu Pathak
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil added 5 comments![]( "Open in Gerrit")
Thanks.
This change would also need to be made in golang.org/x/net/http2 to handle the HTTP/2 server as well.
errRequestHandlerReturned = errors.New("request handler returned")I don't think we should ever export these. Exported errors exist to permit programms to make control-flow decisions based on what error occurred. When would a program need to distinguish between a request handler returning and the connection breaking?
cr.conn.cancelCtx(err)Either use a single catch-all "connection closed" message for connection failures, or prefix this with something useful like `fmt.Errorf("connection read error: %v", err)`
defer cancelCtx(errRequestHandlerReturned)Wrong place for this: This context is cancelled once per connection, but there can be many requests handled per connection. You probably want `w.cancelCtx` below.
w.c.cancelCtx(err)Same as above: Either use a single "connection closed" error message or prefix with "connection write error".
Related details
- Himanshu Pathak
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 4 comments![]( "Open in Gerrit")
Thanks.
This change would also need to be made in golang.org/x/net/http2 to handle the HTTP/2 server as well.
Thanks, will do in this same change set once we reach a final version of the net/http changes.
errRequestHandlerReturned = errors.New("request handler returned")I don't think we should ever export these. Exported errors exist to permit programms to make control-flow decisions based on what error occurred. When would a program need to distinguish between a request handler returning and the connection breaking?
I actually use the exposed Err to determine the log level of related log lines, and this was the reason for proposing this CL. gRPC method failure caused by context cancellation (anywhere in the code) is normally `slog.LevelError` but if the cancellation was caused by a net/http client request cancellation, then the system is working as intended, so it should be Warn or Info.
```
if errors.Is(cause, util.ErrConnectionClosed) {
// Connection was closed by the client, so this Canceled error isn't an error
level = slog.LevelWarn
}
```
If the ErrConnectionClosed is not exported I believe I would have to look at the `.Error()` string to make the distinction between context cancellation caused by net/http or by any other location in the code, isn't it?
cr.conn.cancelCtx(err)Either use a single catch-all "connection closed" message for connection failures, or prefix this with something useful like `fmt.Errorf("connection read error: %v", err)`
Just to be sure what you mean - you are suggesting `cancelCtx(fmt.Errorf("connection read error: %w", err))` correct?
Related details
- Himanshu Pathak
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 1 comment![]( "Open in Gerrit")
errRequestHandlerReturned = errors.New("request handler returned")Sjors GielenI don't think we should ever export these. Exported errors exist to permit programms to make control-flow decisions based on what error occurred. When would a program need to distinguish between a request handler returning and the connection breaking?
I actually use the exposed Err to determine the log level of related log lines, and this was the reason for proposing this CL. gRPC method failure caused by context cancellation (anywhere in the code) is normally `slog.LevelError` but if the cancellation was caused by a net/http client request cancellation, then the system is working as intended, so it should be Warn or Info.
```
if errors.Is(cause, util.ErrConnectionClosed) {
// Connection was closed by the client, so this Canceled error isn't an error
level = slog.LevelWarn
}
```If the ErrConnectionClosed is not exported I believe I would have to look at the `.Error()` string to make the distinction between context cancellation caused by net/http or by any other location in the code, isn't it?
@dn...@google.com friendly bump - could you get back to my reply here?
Related details
- Himanshu Pathak
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 1 comment![]( "Open in Gerrit")
errRequestHandlerReturned = errors.New("request handler returned")Sjors GielenI don't think we should ever export these. Exported errors exist to permit programms to make control-flow decisions based on what error occurred. When would a program need to distinguish between a request handler returning and the connection breaking?
Sjors GielenI actually use the exposed Err to determine the log level of related log lines, and this was the reason for proposing this CL. gRPC method failure caused by context cancellation (anywhere in the code) is normally `slog.LevelError` but if the cancellation was caused by a net/http client request cancellation, then the system is working as intended, so it should be Warn or Info.
```
if errors.Is(cause, util.ErrConnectionClosed) {
// Connection was closed by the client, so this Canceled error isn't an error
level = slog.LevelWarn
}
```If the ErrConnectionClosed is not exported I believe I would have to look at the `.Error()` string to make the distinction between context cancellation caused by net/http or by any other location in the code, isn't it?
@dn...@google.com friendly bump - could you get back to my reply here?
@dn...@google.com respectful bump - still waiting on your thoughts regarding the Errs proposal - would there be any way to get this reviewed? On Github issue #64465 there are two users agreeing with the export of the Errs, could we use this as community input to my suggestion?
Related details
- Himanshu Pathak
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Taiga Nakayama (Gerrit)
Taiga Nakayama added 1 comment![]( "Open in Gerrit")
errRequestHandlerReturned = errors.New("request handler returned")Sjors GielenI don't think we should ever export these. Exported errors exist to permit programms to make control-flow decisions based on what error occurred. When would a program need to distinguish between a request handler returning and the connection breaking?
Sjors GielenI actually use the exposed Err to determine the log level of related log lines, and this was the reason for proposing this CL. gRPC method failure caused by context cancellation (anywhere in the code) is normally `slog.LevelError` but if the cancellation was caused by a net/http client request cancellation, then the system is working as intended, so it should be Warn or Info.
```
if errors.Is(cause, util.ErrConnectionClosed) {
// Connection was closed by the client, so this Canceled error isn't an error
level = slog.LevelWarn
}
```If the ErrConnectionClosed is not exported I believe I would have to look at the `.Error()` string to make the distinction between context cancellation caused by net/http or by any other location in the code, isn't it?
Sjors Gielen@dn...@google.com friendly bump - could you get back to my reply here?
@dn...@google.com respectful bump - still waiting on your thoughts regarding the Errs proposal - would there be any way to get this reviewed? On Github issue #64465 there are two users agreeing with the export of the Errs, could we use this as community input to my suggestion?
Hello @dn...@google.com, @sj...@sjorsgielen.nl
I'm really looking forward to seeing this merged because I would be able to distinguish "context canceled" errors that were caused merely by the users from the others. As @sj...@sjorsgielen.nl said, sometimes we'd like to suppress error logs like that.
Thank you for considering!
Related details
- Himanshu Pathak
- Sjors Gielen
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 1 comment![]( "Open in Gerrit")
@dn...@google.com this CL is waiting for your reply regarding your review on January 31st. In that review, you said the errors added by this CL should not be public. In my reply I proposed to make one error public, because users would be able to special-case that error, and this has been +1'd by multiple community members both here and on GitHub (https://github.com/golang/go/issues/64465). If you could re-review that point and give your opinion on it, I can forwardport this CL to the newest master and also address your other review points. Thank you
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds added 1 comment![]( "Open in Gerrit")
@dn...@google.com this CL is waiting for your reply regarding your review on January 31st. In that review, you said the errors added by this CL should not be public. In my reply I proposed to make one error public, because users would be able to special-case that error, and this has been +1'd by multiple community members both here and on GitHub (https://github.com/golang/go/issues/64465). If you could re-review that point and give your opinion on it, I can forwardport this CL to the newest master and also address your other review points. Thank you
In that review, you said the errors added by this CL should not be public. In my reply I proposed to make one error public, because users would be able to special-case that error, and this has been +1'd by multiple community members both here and on GitHub
Hi Sjors, FWIW, the proposal process includes multiple steps and usually takes a fairly long calendar time. See https://go.dev/s/proposal for details of the process. Also, proposals are essentially created at a faster rate than the rate they can be formally reviewed, and the window for the Go 1.26 dev cycle is getting shorter.
My advice to you would be to focus this CL / PR on the pieces that do not impact any public API and that do need to be part of the proposal process. I think that's similar to [the advice Damien gave](https://github.com/golang/go/issues/64465#issuecomment-2628242698) back in January on the issue.
You can then continue the discussion on GitHub for anything else remaining that does impact public API that needs to go through the formal proposal process.
That said, I haven't looked at the details here, so please just consider this a quick friendly drive by comment.
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds added 2 comments![]( "Open in Gerrit")
Hi Sjors, one other quick piece of advice -- this CL currently has 7 unresolved comments. It's in your interests to resolve as many of those as possible, which then makes it easier for Damien (or whoever) to pick up the review.
As GerritBot said above:
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Also, you probably want to rebase to latest master.
defer cancelCtx(errRequestHandlerReturned)Wrong place for this: This context is cancelled once per connection, but there can be many requests handled per connection. You probably want `w.cancelCtx` below.
As an example, better to address this comment all the way to the point of you being able to click 'Done' or 'Resolved'.
That makes it easier for Damien to pick the review back up when he has time.
(Or if you firmly disagree with the suggestion, reply saying so).
t hepudds (Gerrit)
t hepudds added 1 comment![]( "Open in Gerrit")
t hepudds@dn...@google.com this CL is waiting for your reply regarding your review on January 31st. In that review, you said the errors added by this CL should not be public. In my reply I proposed to make one error public, because users would be able to special-case that error, and this has been +1'd by multiple community members both here and on GitHub (https://github.com/golang/go/issues/64465). If you could re-review that point and give your opinion on it, I can forwardport this CL to the newest master and also address your other review points. Thank you
In that review, you said the errors added by this CL should not be public. In my reply I proposed to make one error public, because users would be able to special-case that error, and this has been +1'd by multiple community members both here and on GitHub
Hi Sjors, FWIW, the proposal process includes multiple steps and usually takes a fairly long calendar time. See https://go.dev/s/proposal for details of the process. Also, proposals are essentially created at a faster rate than the rate they can be formally reviewed, and the window for the Go 1.26 dev cycle is getting shorter.
My advice to you would be to focus this CL / PR on the pieces that do not impact any public API and that do need to be part of the proposal process. I think that's similar to [the advice Damien gave](https://github.com/golang/go/issues/64465#issuecomment-2628242698) back in January on the issue.
You can then continue the discussion on GitHub for anything else remaining that does impact public API that needs to go through the formal proposal process.
That said, I haven't looked at the details here, so please just consider this a quick friendly drive by comment.
Ahh, sorry, I dropped a word while attempting to improve my wording. 🙃
focus this CL / PR on the pieces that do not impact any public API and that do _**not**_ need to be part of the proposal process
Sjors Gielen (Gerrit)
Sjors Gielen added 1 comment![]( "Open in Gerrit")
t hepudds@dn...@google.com this CL is waiting for your reply regarding your review on January 31st. In that review, you said the errors added by this CL should not be public. In my reply I proposed to make one error public, because users would be able to special-case that error, and this has been +1'd by multiple community members both here and on GitHub (https://github.com/golang/go/issues/64465). If you could re-review that point and give your opinion on it, I can forwardport this CL to the newest master and also address your other review points. Thank you
t hepuddsIn that review, you said the errors added by this CL should not be public. In my reply I proposed to make one error public, because users would be able to special-case that error, and this has been +1'd by multiple community members both here and on GitHub
Hi Sjors, FWIW, the proposal process includes multiple steps and usually takes a fairly long calendar time. See https://go.dev/s/proposal for details of the process. Also, proposals are essentially created at a faster rate than the rate they can be formally reviewed, and the window for the Go 1.26 dev cycle is getting shorter.
My advice to you would be to focus this CL / PR on the pieces that do not impact any public API and that do need to be part of the proposal process. I think that's similar to [the advice Damien gave](https://github.com/golang/go/issues/64465#issuecomment-2628242698) back in January on the issue.
You can then continue the discussion on GitHub for anything else remaining that does impact public API that needs to go through the formal proposal process.
That said, I haven't looked at the details here, so please just consider this a quick friendly drive by comment.
Ahh, sorry, I dropped a word while attempting to improve my wording. 🙃
focus this CL / PR on the pieces that do not impact any public API and that do _**not**_ need to be part of the proposal process
Thanks for your feedback @thepud...@gmail.com, I appreciate you taking the time!
I did consider focusing this CL on the pieces that don't need public review - but eventually, this CL work is all intended to be able to special-case on that one specific error (connection closed by client). It doesn't help me much to improve the exact Error() string, if it's still not a case we can catch in our code for special handling. The proposal process to get that error public is the thing that makes it worthwhile for me, and I see on the other comments on GitHub that it is the same for other people running into this.
This is why I focus on getting early feedback on making the error public, and once we're through that, the CL is tiny and I'll be able to work on the feedback quickly.
Do you see my train of thought with this prioritisation or would you suggest I reconsider?
If you follow, do you think the relevant proposal at https://github.com/golang/go/issues/64465#issuecomment-2543377045 is well-defined and in the right place, or do you think I should change something to get that addressed by the right people? Thanks!
Related details
- Damien Neil
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 1 comment![]( "Open in Gerrit")
t hepudds@dn...@google.com this CL is waiting for your reply regarding your review on January 31st. In that review, you said the errors added by this CL should not be public. In my reply I proposed to make one error public, because users would be able to special-case that error, and this has been +1'd by multiple community members both here and on GitHub (https://github.com/golang/go/issues/64465). If you could re-review that point and give your opinion on it, I can forwardport this CL to the newest master and also address your other review points. Thank you
t hepuddsIn that review, you said the errors added by this CL should not be public. In my reply I proposed to make one error public, because users would be able to special-case that error, and this has been +1'd by multiple community members both here and on GitHub
Hi Sjors, FWIW, the proposal process includes multiple steps and usually takes a fairly long calendar time. See https://go.dev/s/proposal for details of the process. Also, proposals are essentially created at a faster rate than the rate they can be formally reviewed, and the window for the Go 1.26 dev cycle is getting shorter.
My advice to you would be to focus this CL / PR on the pieces that do not impact any public API and that do need to be part of the proposal process. I think that's similar to [the advice Damien gave](https://github.com/golang/go/issues/64465#issuecomment-2628242698) back in January on the issue.
You can then continue the discussion on GitHub for anything else remaining that does impact public API that needs to go through the formal proposal process.
That said, I haven't looked at the details here, so please just consider this a quick friendly drive by comment.
Sjors GielenAhh, sorry, I dropped a word while attempting to improve my wording. 🙃
focus this CL / PR on the pieces that do not impact any public API and that do _**not**_ need to be part of the proposal process
Thanks for your feedback @thepud...@gmail.com, I appreciate you taking the time!
I did consider focusing this CL on the pieces that don't need public review - but eventually, this CL work is all intended to be able to special-case on that one specific error (connection closed by client). It doesn't help me much to improve the exact Error() string, if it's still not a case we can catch in our code for special handling. The proposal process to get that error public is the thing that makes it worthwhile for me, and I see on the other comments on GitHub that it is the same for other people running into this.
This is why I focus on getting early feedback on making the error public, and once we're through that, the CL is tiny and I'll be able to work on the feedback quickly.
Do you see my train of thought with this prioritisation or would you suggest I reconsider?
If you follow, do you think the relevant proposal at https://github.com/golang/go/issues/64465#issuecomment-2543377045 is well-defined and in the right place, or do you think I should change something to get that addressed by the right people? Thanks!
Actually upon further consideration, the alternative you propose does feel more attractive. I could focus this CL on simply making the case recognized, even if that doesn't add much value. Then, in second instance the entire proposal is just "making an existing variable public instead of private". That should make review faster as it's clearer what's going to change. If you agree with this, then I'll switch gears and use that approach, instead.
t hepudds (Gerrit)
t hepudds added 1 comment![]( "Open in Gerrit")
t hepudds@dn...@google.com this CL is waiting for your reply regarding your review on January 31st. In that review, you said the errors added by this CL should not be public. In my reply I proposed to make one error public, because users would be able to special-case that error, and this has been +1'd by multiple community members both here and on GitHub (https://github.com/golang/go/issues/64465). If you could re-review that point and give your opinion on it, I can forwardport this CL to the newest master and also address your other review points. Thank you
t hepuddsIn that review, you said the errors added by this CL should not be public. In my reply I proposed to make one error public, because users would be able to special-case that error, and this has been +1'd by multiple community members both here and on GitHub
Hi Sjors, FWIW, the proposal process includes multiple steps and usually takes a fairly long calendar time. See https://go.dev/s/proposal for details of the process. Also, proposals are essentially created at a faster rate than the rate they can be formally reviewed, and the window for the Go 1.26 dev cycle is getting shorter.
My advice to you would be to focus this CL / PR on the pieces that do not impact any public API and that do need to be part of the proposal process. I think that's similar to [the advice Damien gave](https://github.com/golang/go/issues/64465#issuecomment-2628242698) back in January on the issue.
You can then continue the discussion on GitHub for anything else remaining that does impact public API that needs to go through the formal proposal process.
That said, I haven't looked at the details here, so please just consider this a quick friendly drive by comment.
Sjors GielenAhh, sorry, I dropped a word while attempting to improve my wording. 🙃
focus this CL / PR on the pieces that do not impact any public API and that do _**not**_ need to be part of the proposal process
Thanks for your feedback @thepud...@gmail.com, I appreciate you taking the time!
I did consider focusing this CL on the pieces that don't need public review - but eventually, this CL work is all intended to be able to special-case on that one specific error (connection closed by client). It doesn't help me much to improve the exact Error() string, if it's still not a case we can catch in our code for special handling. The proposal process to get that error public is the thing that makes it worthwhile for me, and I see on the other comments on GitHub that it is the same for other people running into this.
This is why I focus on getting early feedback on making the error public, and once we're through that, the CL is tiny and I'll be able to work on the feedback quickly.
Do you see my train of thought with this prioritisation or would you suggest I reconsider?
If you follow, do you think the relevant proposal at https://github.com/golang/go/issues/64465#issuecomment-2543377045 is well-defined and in the right place, or do you think I should change something to get that addressed by the right people? Thanks!
GitHub is the place to discuss whether or not a proposal _should_ be implemented. For example, from https://go.dev/doc/contribute#review
The vast majority of changes require a linked issue that describes the bug or the feature that the change fixes or implements, and **consensus should have been reached on the tracker before proceeding with it. Gerrit reviews do not discuss the merit of the change**, just its implementation.
Or from the [proposal process]( https://go.dev/s/proposal) I linked earlier:
1. The proposal author creates a brief issue describing the proposal.
2. **discussion on the issue tracker** aims to triage the proposal into one of three outcomes: Accept proposal, or decline proposal, or ask for a design doc.
[...]
**After the proposal is accepted or declined** (whether after step 2 or step 4), **implementation work proceeds** in the same way as any other contribution.
-----
If you only want to make a change that goes through the proposal process, I can mark this CL "hold for proposal", and the CL won't be reviewed until the proposal process is complete. (The time to get something in front of the proposal review committee varies significantly, and the queue grows rather than shrinks over time, but my personal guess is that it would not complete in time for Go 1.26 given the development freeze timing).
That said, I guess my personal advice would be to start by following Damien's advice from January. That might help other people, even if it's not your perfect outcome, and the discussion can continue in parallel.
Of course, it's also 1000% fine if you don't want to do that or don't see any value in that. If that's the case, though, maybe state that on the issue, and maybe someone else can pick up the more targeted change to move it forward.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 1 comment![]( "Open in Gerrit")
@thepud...@gmail.com I did read the contribution guide before I started on this CL & proposal. I am/was under the impression that the GitHub issue (the comment I linked to) was collecting community feedback, and otherwise ready to go in front of the proposal review committee (for that one error var to become public). I was waiting for @dn...@google.com to read my review response and mark the GitHub issue as "ready for proposal review committee". Once approved, I could quickly follow through with the CL. Those last steps never happened and therefore both this CL and the associated proposal have been (unnecessarily?) blocked throughout the 1.24 and 1.25 release processes.
I will follow your suggestion though and get this CL through without any proposals necessary. After that, I'll start the proposal over just for the errConnectionClosed to become ErrConnectionClosed since this CL and related GitHub issue have become quite noisy and difficult to review.
Again, thanks very much for taking the time to suggest ways to move this forward, I really appreciate it.
Related details
- Sjors Gielen
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds added 1 comment![]( "Open in Gerrit")
(Our replies effectively crossed.)
What you wrote is reasonable.
I think in general, a proposal like "expose this thing that already exists" is seen as a smaller proposal than "create this thing that does not exist". That said, some chance it could cut the other way, such as "Well, let's see how useful the first change is before we decide about changing the public API".
Taking everything together, my personal advice would still be to follow Damien's advice, at least to start, including because I suspect an API change probably is not happening here anyway for Go 1.26 (based on timing and other factors).
t hepudds (Gerrit)
My probably final comment is just to say that you of course didn't do anything terribly wrong.
It's not always clear to new or new-ish contributors what type of conversation happens in GitHub vs. Gerrit, and I was just trying to give some advice about possible ways to move forward.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Damien Neil
- Sjors Gielen
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Damien Neil
- Sjors Gielen
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Damien Neil
- Sjors Gielen
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Damien Neil
- Sjors Gielen
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 8 comments![]( "Open in Gerrit")
I spotted some possible problems.
These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. Otherwise, please address any problems and update the GitHub PR. When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.
Possible problems detected:
1. It looks like you have a properly formated bug reference, but the convention is to put bug references at the bottom of the commit message, even if a bug is also mentioned in the body of the message.The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Sjors Gielen
The TODO will be removed as a result of review of the proposal.
Done
Sjors GielenThanks.
This change would also need to be made in golang.org/x/net/http2 to handle the HTTP/2 server as well.
Thanks, will do in this same change set once we reach a final version of the net/http changes.
I don't think `golang.org/x/net/http2` needs it: when a connection is closed, it already closes all streams with the `errClientDisconnected` error. I did update this CL to use the same variable name and Error() string, for consistency.
Done
Hi Sjors, one other quick piece of advice -- this CL currently has 7 unresolved comments. It's in your interests to resolve as many of those as possible, which then makes it easier for Damien (or whoever) to pick up the review.
As GerritBot said above:
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Also, you probably want to rebase to latest master.
Done
errRequestHandlerReturned = errors.New("request handler returned")Sjors GielenI don't think we should ever export these. Exported errors exist to permit programms to make control-flow decisions based on what error occurred. When would a program need to distinguish between a request handler returning and the connection breaking?
Sjors GielenI actually use the exposed Err to determine the log level of related log lines, and this was the reason for proposing this CL. gRPC method failure caused by context cancellation (anywhere in the code) is normally `slog.LevelError` but if the cancellation was caused by a net/http client request cancellation, then the system is working as intended, so it should be Warn or Info.
```
if errors.Is(cause, util.ErrConnectionClosed) {
// Connection was closed by the client, so this Canceled error isn't an error
level = slog.LevelWarn
}
```If the ErrConnectionClosed is not exported I believe I would have to look at the `.Error()` string to make the distinction between context cancellation caused by net/http or by any other location in the code, isn't it?
Sjors Gielen@dn...@google.com friendly bump - could you get back to my reply here?
Taiga Nakayama@dn...@google.com respectful bump - still waiting on your thoughts regarding the Errs proposal - would there be any way to get this reviewed? On Github issue #64465 there are two users agreeing with the export of the Errs, could we use this as community input to my suggestion?
Hello @dn...@google.com, @sj...@sjorsgielen.nl
I'm really looking forward to seeing this merged because I would be able to distinguish "context canceled" errors that were caused merely by the users from the others. As @sj...@sjorsgielen.nl said, sometimes we'd like to suppress error logs like that.
Thank you for considering!
I've removed the TODO. As suggested in other comments, for this CL I'll keep the errs private, and once this is in I'll create a proposal to make it public.
Sjors GielenEither use a single catch-all "connection closed" message for connection failures, or prefix this with something useful like `fmt.Errorf("connection read error: %v", err)`
Just to be sure what you mean - you are suggesting `cancelCtx(fmt.Errorf("connection read error: %w", err))` correct?
Done
t hepuddsWrong place for this: This context is cancelled once per connection, but there can be many requests handled per connection. You probably want `w.cancelCtx` below.
As an example, better to address this comment all the way to the point of you being able to click 'Done' or 'Resolved'.
That makes it easier for Damien to pick the review back up when he has time.
(Or if you firmly disagree with the suggestion, reply saying so).
You're right, I misread here. Changed to `defer cancelCtx(nil)` and removed `errRequestHandlerReturned`. There's no particular cancellation cause to pass here; any code that would have a good cancellation cause could call `cancelCtx(...)` itself.
Same as above: Either use a single "connection closed" error message or prefix with "connection write error".
Done
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 1 comment![]( "Open in Gerrit")
@dn...@google.com I am wondering whether a better cancellation cause would be `fmt.Errorf("%w: connection read error: %w", errClientDisconnected, originalErr)` so that both errors.Is(err, io.EOF) and errors.Is(err, errClientDisconnected) are true. But not sure if that's taking it too far for just a cancellation cause, not an actual err returned.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil added 4 comments![]( "Open in Gerrit")
@dn...@google.com this CL is waiting for your reply regarding your review on January 31st. In that review, you said the errors added by this CL should not be public. In my reply I proposed to make one error public, because users would be able to special-case that error, and this has been +1'd by multiple community members both here and on GitHub (https://github.com/golang/go/issues/64465). If you could re-review that point and give your opinion on it, I can forwardport this CL to the newest master and also address your other review points. Thank you
Apologies for the delay in responding.
As thepudds says, changes to the exported API of a package need to go through the proposal process (https://go.dev/s/proposal). The proposal process is very lengthy, we have limited bandwidth to review new proposals, and proposals get written faster than they can be reviewed so in general it is quite difficult to add new exported APIs. Nobody is especially satisfied by this situation, but there aren't any easy fixes to it.
But a CL that adds an exported symbol to a package may not be approved unless it has an accompanying approved proposal, no matter how many +1s it may have on GitHub.
(In the interest of full disclosure, I currently sit on the proposal committee. However, that doesn't give me the authority to approve a CL adding new APIs that haven't gone through the proposal process.)
I'll comment on #64465 about the specifics of adding exported symbols here.
err = errClientDisconnected`io.EOF` indicates that the peer closed this connection, while `net.ErrClosed` indicates that we closed it locally. Blending these together seems like it loses valuable information.
`io.EOF` isn't a very informative user-facing error, so I think it does make sense to translate it to something clearer:
```
if err == io.EOF {
err = errClientDisconnected
}
```
For the case where we close the connection locally, we should try to provide information on why are closing the connection. I think the only case where a connection with an active request on it can be closed is `Server.Close`, so we could explicitly cancel the connection contexts there:
```
// In Server.Close:
for c := range s.activeConn {
c.cancelCtx(errors.New("connection closed by server shutdown"))
c.rwc.Close()
delete(s.activeConn, c)
}
```
cr.conn.cancelCtx(fmt.Errorf("connection read error: %w", err))Use %v.
I don't see any non-racy way for a caller to make use of a wrapped error here. If a connection encounters an error, is the wrapped error from the Read call or the Write call? If someone comes to depend on the wrapped error being some value, are we then unable to give them a more informative cause (such as "connection closed by server shutdown")?
w.c.cancelCtx(fmt.Errorf("connection write error: %w", err))We shouldn't be able to see `io.EOF` here.
net.ErrClosed (as I described above) only happens when we close the connection locally. We should call cancelCtx with a more informative error in the cases where this happens to a live connection (I think only `Server.Close`).
We can then leave the error unaltered here: Just:
```
w.c.cancelCtx(fmt.Errorf("connection write error: %v", err))
```
...and don't try to add any additional interpretation to `err`.
(Also, use %v, as discussed above.)
Related details
- Sjors Gielen
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Damien Neil
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Damien Neil
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 1 comment![]( "Open in Gerrit")
w.c.cancelCtx(fmt.Errorf("connection write error: %w", err))We shouldn't be able to see `io.EOF` here.
net.ErrClosed (as I described above) only happens when we close the connection locally. We should call cancelCtx with a more informative error in the cases where this happens to a live connection (I think only `Server.Close`).
We can then leave the error unaltered here: Just:
```
w.c.cancelCtx(fmt.Errorf("connection write error: %v", err))
```...and don't try to add any additional interpretation to `err`.
(Also, use %v, as discussed above.)
Sjors GielenTo add to my earlier comment: I did see these write errors pop up on my test server & client on various types of closures during various phases, so it is important to add ErrClientDisconnected here, too (and likewise, important to use %w).
To clarify: I did not see io.EOF, but I did see ECONNREFUSED and EPIPE. I have removed io.EOF from the error handling in Write() accordingly.
Related details
- Damien Neil
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Damien Neil
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 1 comment![]( "Open in Gerrit")
Apologies - last patch set fixes the test failure.
Related details
- Damien Neil
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 1 comment![]( "Open in Gerrit")
The test failure is not a flake, it's due to the new references to `syscall.ECONNREFUSED` and `syscall.EPIPE` which are undefined on plan9:
```
# net/http
net/http/server.go:784:35: undefined: syscall.ECONNRESET
net/http/server.go:784:73: undefined: syscall.EPIPE
net/http/server.go:4093:29: undefined: syscall.ECONNRESET
net/http/server.go:4093:67: undefined: syscall.EPIPE
```
I'm not sure how to handle these. Could add an `isDisconnect(target err)` in a separate file with a build tag, but that seems crude? What do you think?
Related details
- Damien Neil
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds added 1 comment![]( "Open in Gerrit")
The test failure is not a flake, it's due to the new references to `syscall.ECONNREFUSED` and `syscall.EPIPE` which are undefined on plan9:
```
# net/http
net/http/server.go:784:35: undefined: syscall.ECONNRESET
net/http/server.go:784:73: undefined: syscall.EPIPE
net/http/server.go:4093:29: undefined: syscall.ECONNRESET
net/http/server.go:4093:67: undefined: syscall.EPIPE
```I'm not sure how to handle these. Could add an `isDisconnect(target err)` in a separate file with a build tag, but that seems crude? What do you think?
Hi Sjors, I'm not sure what is the best solution (and I'm trying to avoid nerd-sniping myself into looking at the details 😊), but taking advantage of build tags is probably reasonable (maybe via an existing file, if there is one). Personally I would in `src/net` do a recursive grep for `plan9` and `find . -name "*plan9*"` or similar, look at 3-4 examples, and then I would make a guess based on that.
Separately, my high-level suggestion would be you make your best guess at a solution and then update the CL.
That way, Damien can hopefully see a passing set of tests here when he has time to circle back and maybe he says your solution is OK or maybe not, but overall that can make it more efficient for everyone and can cut down on total calendar time and reduce "turns" in the conversation...
(And it's also of course fine to ask for a suggested solution too, as you did).
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 1 comment![]( "Open in Gerrit")
t hepuddsThe test failure is not a flake, it's due to the new references to `syscall.ECONNREFUSED` and `syscall.EPIPE` which are undefined on plan9:
```
# net/http
net/http/server.go:784:35: undefined: syscall.ECONNRESET
net/http/server.go:784:73: undefined: syscall.EPIPE
net/http/server.go:4093:29: undefined: syscall.ECONNRESET
net/http/server.go:4093:67: undefined: syscall.EPIPE
```I'm not sure how to handle these. Could add an `isDisconnect(target err)` in a separate file with a build tag, but that seems crude? What do you think?
Hi Sjors, I'm not sure what is the best solution (and I'm trying to avoid nerd-sniping myself into looking at the details 😊), but taking advantage of build tags is probably reasonable (maybe via an existing file, if there is one). Personally I would in `src/net` do a recursive grep for `plan9` and `find . -name "*plan9*"` or similar, look at 3-4 examples, and then I would make a guess based on that.
Separately, my high-level suggestion would be you make your best guess at a solution and then update the CL.
That way, Damien can hopefully see a passing set of tests here when he has time to circle back and maybe he says your solution is OK or maybe not, but overall that can make it more efficient for everyone and can cut down on total calendar time and reduce "turns" in the conversation...
(And it's also of course fine to ask for a suggested solution too, as you did).
Thanks @thepudds - there weren't many non-test examples, and none related to errors, so I've made my best guess at a solution and added it to the CL as you suggested :-)
Could you request another test run? (There's no way for me to do this, I believe, is there?)
Related details
- Damien Neil
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Damien Neil
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds added 3 comments![]( "Open in Gerrit")
// Copyright 2015 The Go Authors. All rights reserved.New files should have the current year for copyright.
// +build plan9You should use the modern form of build constraints:
"errors"
"syscall"These imports are not used in this file.
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds added 2 comments![]( "Open in Gerrit")
// +build plan9You should use the modern form of build constraints:
Actually, this one might be redundant with the filename ending `_plan9.go`?
If so, probably better to remove (unless that seems to be the pattern in net/http already; did not check myself).
Note you should be able to build locally, like:
```
$ GOOS=plan9 go build net/http
```
// +build !plan9This one should use the modern form of build constraints:
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Damien Neil
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Damien Neil
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 3 comments![]( "Open in Gerrit")
New files should have the current year for copyright.
Done
t hepuddsYou should use the modern form of build constraints:
Actually, this one might be redundant with the filename ending `_plan9.go`?
If so, probably better to remove (unless that seems to be the pattern in net/http already; did not check myself).
Note you should be able to build locally, like:
```
$ GOOS=plan9 go build net/http
```
Done. Good point about GOOS, I didn't think of that.
These imports are not used in this file.
Done
Related details
- Damien Neil
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sjors Gielen (Gerrit)
Sjors Gielen added 1 comment![]( "Open in Gerrit")
// +build !plan9Sjors GielenThis one should use the modern form of build constraints:
Done
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jes Cok (Gerrit)
Jes Cok added 1 comment![]( "Open in Gerrit")
if errors.Is(err, io.EOF) {According to the doc of io.EOF, I think it should be ==. Not familiar with this code, maybe I’m missing something.
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds added 1 comment![]( "Open in Gerrit")
Hi Sjors, what is your thinking regarding whether there can be a test that accompanies these changes?
Usually changes in behavior should be tested, including so that some future change by someone else does not break the new behavior, though maybe there's some reason that does not apply here.
I think you mentioned you wrote a local test server and described it here:
https://go-review.googlesource.com/c/go/+/636235/comment/70fb6319_9f29b34c/
FWIW, there are plenty of tests in the stdlib that spin up a small test server, though I don't know if that is the best approach here.
You could look through net/http/serve_test.go or clientserver_test.go or similar to see if there is an example you could follow. (I'm not sure those are the best starting spots, but you can probably find some useful examples if you poke around the tests there and elsewhere).
That said, if you suspect it is not feasible or not worthwhile for some reason, you could wait for Damien or someone else to weigh in on testing before going too far in your investigation, but it would be good to at least add a brief reply here with your thoughts on whether a test can reasonably be checked in here. If it's not clear that a test could be checked in, probably worth at least a quick update to the commit message with a brief summary of the current testing approach (which otherwise might be lost in the now many comments).
Sjors Gielen (Gerrit)
Sjors Gielen added 2 comments![]( "Open in Gerrit")
Hi Sjors, what is your thinking regarding whether there can be a test that accompanies these changes?
Usually changes in behavior should be tested, including so that some future change by someone else does not break the new behavior, though maybe there's some reason that does not apply here.
I think you mentioned you wrote a local test server and described it here:
https://go-review.googlesource.com/c/go/+/636235/comment/70fb6319_9f29b34c/FWIW, there are plenty of tests in the stdlib that spin up a small test server, though I don't know if that is the best approach here.
You could look through net/http/serve_test.go or clientserver_test.go or similar to see if there is an example you could follow. (I'm not sure those are the best starting spots, but you can probably find some useful examples if you poke around the tests there and elsewhere).
That said, if you suspect it is not feasible or not worthwhile for some reason, you could wait for Damien or someone else to weigh in on testing before going too far in your investigation, but it would be good to at least add a brief reply here with your thoughts on whether a test can reasonably be checked in here. If it's not clear that a test could be checked in, probably worth at least a quick update to the commit message with a brief summary of the current testing approach (which otherwise might be lost in the now many comments).
I think the testing setup I wrote can be transformed into a number of good tests, indeed. It does cover OS behaviour (regarding ECONNRESET / EPIPE) and is racy (errors can occur during read and write) but I think it's feasible to make it reliable and not flaky, and worthwhile to spend time on it.
if errors.Is(err, io.EOF) {According to the doc of io.EOF, I think it should be ==. Not familiar with this code, maybe I’m missing something.
The way I interpret the io.EOF documentation, it only specifies what Read must return (in case users do compare with ==), not that comparison should be done with ==?
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Damien Neil
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Damien Neil
- Jes Cok
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil added 2 comments![]( "Open in Gerrit")
cr.conn.cancelCtx(fmt.Errorf("connection read error: %w", err))I'm sorry, but this seems like far too much complexity for too little gain.
All we need here is:
```
cr.conn.cancelCtx(fmt.Errorf("connection read error: %v", err))
```
There was a read error on the connection, and therefore we closed it. Trying to sort read errors into "client disconnected" and "there was a read error, but the client didn't disconnect, it just...isn't connected any more" doesn't seem useful.
And use %v. It is not possible to use the underlying read error in a non-racy fashion, since in almost any real-world scenario the read and write errors will be racing each other.
w.c.cancelCtx(fmt.Errorf("connection write error: %w", err))Here as well: Trying to sort write errors into "disconnected" vs. "not connected any more but we aren't calling it disconnected" doesn't seem useful.
Just:
```
w.c.cancelCtx(fmt.Errorf("connection write error: %v", err))
```
- Jes Cok
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil added 1 comment![]( "Open in Gerrit")
Sjors GielenHi Sjors, what is your thinking regarding whether there can be a test that accompanies these changes?
Usually changes in behavior should be tested, including so that some future change by someone else does not break the new behavior, though maybe there's some reason that does not apply here.
I think you mentioned you wrote a local test server and described it here:
https://go-review.googlesource.com/c/go/+/636235/comment/70fb6319_9f29b34c/FWIW, there are plenty of tests in the stdlib that spin up a small test server, though I don't know if that is the best approach here.
You could look through net/http/serve_test.go or clientserver_test.go or similar to see if there is an example you could follow. (I'm not sure those are the best starting spots, but you can probably find some useful examples if you poke around the tests there and elsewhere).
That said, if you suspect it is not feasible or not worthwhile for some reason, you could wait for Damien or someone else to weigh in on testing before going too far in your investigation, but it would be good to at least add a brief reply here with your thoughts on whether a test can reasonably be checked in here. If it's not clear that a test could be checked in, probably worth at least a quick update to the commit message with a brief summary of the current testing approach (which otherwise might be lost in the now many comments).
I think the testing setup I wrote can be transformed into a number of good tests, indeed. It does cover OS behaviour (regarding ECONNRESET / EPIPE) and is racy (errors can occur during read and write) but I think it's feasible to make it reliable and not flaky, and worthwhile to spend time on it.
Damien Neil@thepudds added two tests in my last commit, based on the tiny test server that I wrote earlier. I've decided to expose both errClientDisconnected and the underlying network error if any (e.g. ECONNRESET/EPIPE), so that the user could handle it further, e.g. for metrics or whatever.
FWIW, I'm reasonably okay with this change not having a test because it doesn't (or shouldn't) have any observable changes to behavior other than a purely informational change to the context cause.
That said, if I was writing a test it would be:
1. Create a server (probably using `newClientServerTest`).
2. Send a request to it.
3. The request handler blocks on the request context.
4. Stop the server.
5. Verify that the request context is canceled with a cause indicating server shutdown.
Related details
- Jes Cok
- Sjors Gielen
- t hepudds