-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
client: do not timeout when wait is true #4254
Conversation
would it be worthwhile to have an e2e test for this / proxying? |
@heyitsanthony Good idea. Will add e2e test for this case. Thanks, |
@@ -442,9 +443,16 @@ func (c *simpleHTTPClient) Do(ctx context.Context, act httpAction) (*http.Respon | |||
return nil, nil, err | |||
} | |||
|
|||
isWait := false | |||
if req != nil { | |||
if req.URL != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&&
instead of nesting if's
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok will fix. thanks
lgtm |
@heyitsanthony Just added e2e test that only fails without this patch.
With this patch, e2e test passes: PTAL. Thanks! |
@@ -42,6 +44,7 @@ func TestBasicOpsNoTLS(t *testing.T) { | |||
isPeerTLS: false, | |||
initialToken: "new", | |||
}, | |||
etcdProcessBasePort, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why have a new base port argument? the same ports were reusable across tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemed like test was hanging for watch cases when using the same ports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds broken; a process might be hanging
@gyuho This change seems to be OK. But please update the documentation accordingly. defer to @heyitsanthony on the testing changes. |
@xiang90 Now understand what you were talking about. I will make the comment clearer. And will update tests addressing Anthony's feedback. Thanks, |
go func() { | ||
defer wg.Done() | ||
args = append(args, "watch", key) | ||
proc, err := spawnCmd(args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs a proc.Close()
(spawnWithExpect
will do it for you)
Basically the whole long polling behavior is broken for the proxy case when proxy does not forward response header or response in time. The request header is designed for watch request in the first place. Or we have no way to know if the remote machine is hang or not. I am not even sure if we still need request header timeout or not. |
}() | ||
|
||
endpoint, be := "", "" | ||
for _, p := range epc.procs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this loop should probably be replaced by calling methods like (e *EtcdProcessCluster) Members() []*etcdProcess
and (e *EtcdProcessCluster) Proxies() []*etcdProcess
c595075
to
bc58090
Compare
Current V2 watch waits by encoding URL with wait=true. When a client sets 'no-sync', it requests directly to proxy and the proxy redirects it by cloning the request object, which leads to cancel the original request when it times out and the cloned request gets closed prematurely. This fixes coreos#3894 by querying the original client request in order to not use context timeout when 'wait=true'.
@heyitsanthony Could you take a look at |
lgtm |
Thanks! |
client: do not timeout when wait is true
Current V2 watch waits by encoding URL with wait=true.
When a client sets 'no-sync', it requests directly to
proxy and the proxy redirects it by cloning the request
object, which leads to cancel the original request when
it times out and the cloned request gets closed prematurely.
This fixes #3894 by querying
the original client request in order to not use context timeout
when 'wait=true'.