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

Verify Endpoint.Info() before accessing it #17703

Merged
merged 1 commit into from Nov 5, 2015
Merged

Verify Endpoint.Info() before accessing it #17703

merged 1 commit into from Nov 5, 2015

Conversation

aboch
Copy link
Contributor

@aboch aboch commented Nov 4, 2015

  • During concurrent stress operations in multi-host environment, it is possible to run into a nil EndpointInfo when running docker network ls. It is a valid situation. It just means the endpoint is no longer available in the datastore.

Signed-off-by: Alessandro Boch aboch@docker.com

- During concurrent operations in multihost environment,
  it is possible that the implementer of `EndpointInfo`
  is nil. It simply means the endpoint is no longer
  available in the datastore.

Signed-off-by: Alessandro Boch <aboch@docker.com>
@calavera
Copy link
Contributor

calavera commented Nov 4, 2015

LGTM

@cpuguy83
Copy link
Member

cpuguy83 commented Nov 4, 2015

This is OK, but I think it's better to return a nil, err instead of nil and then always checking if the thing is nil.
The error type isn't necessarily for real/hard error cases that we can't come back from, it can be used to signify these types of things, ie "this thing you asked for doesn't actually exist".
This would prevent cases like this where it's only returning one thing so we just assume we're good to use it.

@aboch
Copy link
Contributor Author

aboch commented Nov 4, 2015

@cpuguy83 Makes sense, but that would be a public API change.

@cpuguy83
Copy link
Member

cpuguy83 commented Nov 4, 2015

public API of what?

@aboch
Copy link
Contributor Author

aboch commented Nov 5, 2015

Libnetwork package API. We have been trying not to change existing API in libentwork, not only the ones interfacing with remote drivers. Sure the non driver interfacing API can be changed, given daemon is probably going to be the only client.

@cpuguy83
Copy link
Member

cpuguy83 commented Nov 5, 2015

LGTM.

Yes, I get we don't want to change the driver API. Package API should be fine to change as this should be versioned by checkouts.

cpuguy83 added a commit that referenced this pull request Nov 5, 2015
Verify Endpoint.Info() before accessing it
@cpuguy83 cpuguy83 merged commit f18c5e9 into moby:master Nov 5, 2015
@thaJeztah
Copy link
Member

@cpuguy83 @aboch please check if PRs need to be added to the 1.9.1 milestone when merging, so that we don't forget.

@aboch
Copy link
Contributor Author

aboch commented Nov 5, 2015

@thaJeztah Yes, please. This should be added to 1.9.1.

@cpuguy83 cpuguy83 added this to the 1.9.1 milestone Nov 5, 2015
@thaJeztah
Copy link
Member

Thanks! If you see another PR that should be considered, feel free to ping me or another maintainer ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants