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

fix: avoid panics when checking container state and container.raw is nil #635

Merged
merged 2 commits into from Nov 24, 2022

Conversation

mdelapenya
Copy link
Collaborator

@mdelapenya mdelapenya commented Nov 23, 2022

What does this PR do?

This PR is fixing how the container State function is handling errors when an edge case takes place. We checked that the raw representation of the container is used internally in the State function, returning it when an error is raised by the Docker client when inspecting a container.

An easy way to reproduce the error described in #540 is to proceed as the test we are adding in this PR: 1) creating a container, 2) terminate it, 3) call container.State function

We noticed the error handling was incorrect in that case when a user calls the State function after termination: it's weird to do so, but the library should protect users for commiting this kind of mistakes. Therefore, we are simply checking if the raw representation is NIL before accessing it, which is the reported error: if raw is nil, then we return nil (the same response from Docker), otherwise return the already "cached" raw.State.

We could have simply returned nil, but we decided to reduce the changeset in this PR and maybe elaborate a follow-up PR with a more consistent change, which could be optimizing the Terminate function to clear the raw representation of the container.

Why is it important?

Consistent error handling when a container is removed (using the public API or because the underlying machine kills the container because of lack of resources).

Related issues

Follow-ups

As mentioned above, I recommend revisiting the Terminate function and clean up all the "state" that lives in the container struct, so that any operation accesing the container returns a clean state. This change could lead to a refactor in any access to the raw representation.

@mdelapenya mdelapenya requested a review from a team as a code owner November 23, 2022 12:43
@mdelapenya mdelapenya added the bug An issue with the library label Nov 23, 2022
@mdelapenya mdelapenya self-assigned this Nov 23, 2022
@mdelapenya mdelapenya requested a review from a team November 23, 2022 12:43
@mdelapenya
Copy link
Collaborator Author

@doubleknd26 could you please take a look at this PR? 🙏 I think it solves the issue you reported in #540

Copy link

@kkdeok kkdeok left a comment

Choose a reason for hiding this comment

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

@mdelapenya LGTM! I'm sure it will solve the issue. Thank you for your work!

@mdelapenya mdelapenya merged commit 559a1b0 into testcontainers:main Nov 24, 2022
@mdelapenya mdelapenya deleted the container-state branch November 25, 2022 12:52
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Nov 29, 2022
* main:
  Add toxiproxy example (testcontainers#643)
  Add spanner example (testcontainers#642)
  chore: sync governance files (testcontainers#641)
  Add pubsub example (testcontainers#640)
  chore: adjust generator for the docs site (testcontainers#639)
  Add datastore example (testcontainers#638)
  Add firestore example (testcontainers#637)
  fix: avoid panics when checking container state and container.raw is nil (testcontainers#635)
  feat: provide a tool to generate examples from code (#618)
  chore: bump version in mkdocs (testcontainers#630)
  docs: remove code snippets from main README (testcontainers#631)
  docs: document replace directive for Docker Compose (testcontainers#632)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue with the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: NPE when call DockerContainer.State()
2 participants