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

*: Use http.Request.WithContext instead of Cancel #7269

Merged
merged 1 commit into from
Feb 2, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 18 additions & 36 deletions lease/leasehttp/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package leasehttp

import (
"bytes"
"context"
"errors"
"fmt"
"io/ioutil"
Expand All @@ -26,7 +27,6 @@ import (
"github.com/coreos/etcd/lease"
"github.com/coreos/etcd/lease/leasepb"
"github.com/coreos/etcd/pkg/httputil"
"golang.org/x/net/context"
)

var (
Expand Down Expand Up @@ -202,45 +202,27 @@ func TimeToLiveHTTP(ctx context.Context, id lease.LeaseID, keys bool, url string
}
req.Header.Set("Content-Type", "application/protobuf")

cancel := httputil.RequestCanceler(req)
req = req.WithContext(ctx)

cc := &http.Client{Transport: rt}
var b []byte
// buffer errc channel so that errc don't block inside the go routinue
errc := make(chan error, 2)
go func() {
resp, err := cc.Do(req)
if err != nil {
errc <- err
return
}
b, err = readResponse(resp)
if err != nil {
errc <- err
return
}
if resp.StatusCode == http.StatusRequestTimeout {
errc <- ErrLeaseHTTPTimeout
return
}
if resp.StatusCode == http.StatusNotFound {
errc <- lease.ErrLeaseNotFound
return
}
if resp.StatusCode != http.StatusOK {
errc <- fmt.Errorf("lease: unknown error(%s)", string(b))
return
}
errc <- nil
}()
select {
case derr := <-errc:
if derr != nil {
return nil, derr
}
case <-ctx.Done():
cancel()
return nil, ctx.Err()
resp, err := cc.Do(req)
if err != nil {
return nil, err
}
b, err = readResponse(resp)
if err != nil {
return nil, err
}
if resp.StatusCode == http.StatusRequestTimeout {
return nil, ErrLeaseHTTPTimeout
}
if resp.StatusCode == http.StatusNotFound {
return nil, lease.ErrLeaseNotFound
}
if resp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("lease: unknown error(%s)", string(b))
}

lresp := &leasepb.LeaseInternalResponse{}
Expand Down
9 changes: 0 additions & 9 deletions pkg/httputil/httputil.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,6 @@ import (
"net/http"
)

func RequestCanceler(req *http.Request) func() {
ch := make(chan struct{})
req.Cancel = ch

return func() {
close(ch)
}
}

// GracefulClose drains http.Response.Body until it hits EOF
// and closes it. This prevents TCP/TLS connections from closing,
// therefore available for reuse.
Expand Down
8 changes: 4 additions & 4 deletions proxy/httpproxy/reverse.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package httpproxy

import (
"bytes"
"context"
"fmt"
"io"
"io/ioutil"
Expand All @@ -24,11 +25,9 @@ import (
"net/url"
"strings"
"sync/atomic"

"time"

"github.com/coreos/etcd/etcdserver/api/v2http/httptypes"
"github.com/coreos/etcd/pkg/httputil"
"github.com/coreos/pkg/capnslog"
)

Expand Down Expand Up @@ -110,15 +109,16 @@ func (p *reverseProxy) ServeHTTP(rw http.ResponseWriter, clientreq *http.Request
var requestClosed int32
completeCh := make(chan bool, 1)
closeNotifier, ok := rw.(http.CloseNotifier)
cancel := httputil.RequestCanceler(proxyreq)
ctx, cancel := context.WithCancel(context.Background())
proxyreq = proxyreq.WithContext(ctx)
defer cancel()
if ok {
closeCh := closeNotifier.CloseNotify()
go func() {
select {
case <-closeCh:
atomic.StoreInt32(&requestClosed, 1)
plog.Printf("client %v closed request prematurely", clientreq.RemoteAddr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should still keep this cancel. the go routine exit at line 117 will not trigger the defer func at line 114.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiang90 its not required there. It will still work fine. The reason being the code is written in such a way that on either of these select triggered, the method will exit and also cancel will get called. The defer func after the select causes it to end i think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i read the code again. what you said makes sense. Thanks.

cancel()
case <-completeCh:
}
}()
Expand Down