/juju

api: impose stricter deadline on api.Open #7507

Merged
merged 1 commit into from Jun 16, 2017

Conversation

Reviewers

@axw axw

@mhilton mhilton

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@rogpeppe @jujubot @axw @mhilton
@rogpeppe
Owner

rogpeppe commented Jun 15, 2017

Currently if you call api.Open with a given timeout,
it might exceed that timeout if the connect succeeds
but (for whatever reason) the Login API call doesn't return.

This PR imposes the timeout on the login too, so clients
that care can be more sure that the api.Open will actually
return soon after a timeout.

To avoid changing the useful semantic (relied on by many tests)
that the zero DialOpts will log in without any particular timeout,
we change the semantics slightly, such that a zero timeout
means no timeout and a zero RetryDelay means "don't retry",
which means that the zero DialOpts means pretty much what
it currently means.

QA no regressions. This should not affect normal behaviour.
@mhilton

mhilton approved these changes Jun 15, 2017
View changes

LGTM

@rogpeppe
Owner

rogpeppe commented Jun 15, 2017

!!build!!
@axw

axw approved these changes Jun 16, 2017
View changes

@rogpeppe
api: impose stricter deadline on api.Open 
Currently if you call api.Open with a given timeout,
it might exceed that timeout if the connect succeeds
but (for whatever reason) the Login API call doesn't return.

This PR imposes the timeout on the login too, so clients
that care can be more sure that the api.Open will actually
return soon after a timeout.

To avoid changing the useful semantic (relied on by many tests)
that the zero DialOpts will log in without any particular timeout,
we change the semantics slightly, such that a zero timeout
means no timeout and a zero RetryDelay means "don't retry",
which means that the zero DialOpts means pretty much what
it currently means.
a867fcf
@rogpeppe
Owner

rogpeppe commented Jun 16, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Jun 16, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 8608324 into juju:develop Jun 16, 2017
1 check passed

1 check passed

github-check-merge-juju Ran tests against PR. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details

wallyworld added a commit to wallyworld/juju that referenced this pull request Jun 22, 2017

@jujubot @wallyworld
Merge pull request #7507 from rogpeppe/167-api-open-login-timeout 
api: impose stricter deadline on api.Open

Currently if you call api.Open with a given timeout,
it might exceed that timeout if the connect succeeds
but (for whatever reason) the Login API call doesn't return.

This PR imposes the timeout on the login too, so clients
that care can be more sure that the api.Open will actually
return soon after a timeout.

To avoid changing the useful semantic (relied on by many tests)
that the zero DialOpts will log in without any particular timeout,
we change the semantics slightly, such that a zero timeout
means no timeout and a zero RetryDelay means "don't retry",
which means that the zero DialOpts means pretty much what
it currently means.

QA no regressions. This should not affect normal behaviour.
52fbc38

jujubot added a commit that referenced this pull request Jun 22, 2017

@jujubot
Merge pull request #7544 from wallyworld/backport-pr7507 
Merge pull request #7507 from rogpeppe/167-api-open-login-timeout

api: impose stricter deadline on api.Open

Currently if you call api.Open with a given timeout,
it might exceed that timeout if the connect succeeds
but (for whatever reason) the Login API call doesn't return.

This PR imposes the timeout on the login too, so clients
that care can be more sure that the api.Open will actually
return soon after a timeout.

To avoid changing the useful semantic (relied on by many tests)
that the zero DialOpts will log in without any particular timeout,
we change the semantics slightly, such that a zero timeout
means no timeout and a zero RetryDelay means "don't retry",
which means that the zero DialOpts means pretty much what
it currently means.

QA no regressions. This should not affect normal behaviour.

## Please provide the following details to expedite Pull Request review:

----

## Description of change

Why is this change needed?

## QA steps

How do we verify that the change works?

## Documentation changes

Does it affect current user workflow? CLI? API?

## Bug reference

Does this change fix a bug? Please add a link to it.
77eeb0d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment