-
-
Notifications
You must be signed in to change notification settings - Fork 286
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: Throw exception if Docker resource does not exist instead of silently ignoring it #1254
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
a6b414c
to
472741f
Compare
While experimenting with Podman I tried to run tests with the following settings: ```sh cd testcontainers-dotnet/tests/Testcontainers.PostgreSql.Tests export DOCKER_HOST=unix://${HOME}/.local/share/containers/podman/machine/podman.sock export TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE=/run/user/501/podman/podman.sock dotnet test --filter ConnectionStateReturnsOpen ``` This lead to a `NullReferenceException` ``` [xUnit.net 00:00:01.52] Testcontainers.PostgreSql.PostgreSqlContainerTest.ConnectionStateReturnsOpen [FAIL] Failed Testcontainers.PostgreSql.PostgreSqlContainerTest.ConnectionStateReturnsOpen [1 ms] Error Message: System.NullReferenceException : Object reference not set to an instance of an object. Stack Trace: at DotNet.Testcontainers.Containers.DockerContainer.get_State() in ~/testcontainers-dotnet/src/Testcontainers/Containers/DockerContainer.cs:line 177 at DotNet.Testcontainers.Configurations.UntilContainerIsRunning.UntilAsync(IContainer container) in ~/testcontainers-dotnet/src/Testcontainers/Configurations/WaitStrategies/UntilContainerIsRunning.cs:line 12 at DotNet.Testcontainers.Configurations.WaitStrategy.UntilAsync(IContainer container, CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Configurations/WaitStrategies/WaitStrategy.cs:line 102 at DotNet.Testcontainers.Containers.DockerContainer.CheckReadinessAsync(WaitStrategy waitStrategy, CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/DockerContainer.cs:line 534 at DotNet.Testcontainers.Configurations.WaitStrategy.<>c__DisplayClass24_0.<<WaitUntilAsync>g__UntilAsync|0>d.MoveNext() in ~/testcontainers-dotnet/src/Testcontainers/Configurations/WaitStrategies/WaitStrategy.cs:line 184 --- End of stack trace from previous location --- at DotNet.Testcontainers.Configurations.WaitStrategy.WaitUntilAsync(Func`1 wait, TimeSpan interval, TimeSpan timeout, Int32 retries, CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Configurations/WaitStrategies/WaitStrategy.cs:line 213 at DotNet.Testcontainers.Containers.DockerContainer.CheckReadinessAsync(IEnumerable`1 waitStrategies, CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/DockerContainer.cs:line 552 at DotNet.Testcontainers.Containers.DockerContainer.UnsafeStartAsync(CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/DockerContainer.cs:line 479 at DotNet.Testcontainers.Containers.DockerContainer.StartAsync(CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/DockerContainer.cs:line 282 at DotNet.Testcontainers.Containers.ResourceReaper.GetAndStartNewAsync(Guid sessionId, IDockerEndpointAuthenticationConfiguration dockerEndpointAuthConfig, IImage resourceReaperImage, IMount dockerSocket, ILogger logger, Boolean requiresPrivilegedMode, TimeSpan initTimeout, CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/ResourceReaper.cs:line 219 at DotNet.Testcontainers.Containers.ResourceReaper.GetAndStartNewAsync(Guid sessionId, IDockerEndpointAuthenticationConfiguration dockerEndpointAuthConfig, IImage resourceReaperImage, IMount dockerSocket, ILogger logger, Boolean requiresPrivilegedMode, TimeSpan initTimeout, CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/ResourceReaper.cs:line 243 at DotNet.Testcontainers.Containers.ResourceReaper.GetAndStartDefaultAsync(IDockerEndpointAuthenticationConfiguration dockerEndpointAuthConfig, ILogger logger, Boolean isWindowsEngineEnabled, CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/ResourceReaper.cs:line 135 at DotNet.Testcontainers.Clients.TestcontainersClient.RunAsync(IContainerConfiguration configuration, CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Clients/TestcontainersClient.cs:line 294 at DotNet.Testcontainers.Containers.DockerContainer.UnsafeCreateAsync(CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/DockerContainer.cs:line 415 at DotNet.Testcontainers.Containers.DockerContainer.StartAsync(CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/DockerContainer.cs:line 279 ``` The problem with this configuration is that the reaper container doesn't start but this is undiagnosable because of the swallowed exception. So instead of swallowing the exception in `ByIdAsync` the `DockerApiException` is now caught inside `ExistsWithIdAsync` instead. Although not obvious, the exception after this commit is better and can put you on the right track to understand that the issue is about the reaper container not starting properly. ``` [xUnit.net 00:00:01.39] Testcontainers.PostgreSql.PostgreSqlContainerTest.ConnectionStateReturnsOpen [FAIL] Failed Testcontainers.PostgreSql.PostgreSqlContainerTest.ConnectionStateReturnsOpen [1 ms] Error Message: Docker.DotNet.DockerContainerNotFoundException : Docker API responded with status code=NotFound, response={"cause":"no such container","message":"no container with name or ID \"91ce0224526fddb896f2163e764e03891fce19120059fdbd04c2112cbab076a6\" found: no such container","response":404} Stack Trace: at Docker.DotNet.ContainerOperations.<>c.<.cctor>b__30_0(HttpStatusCode statusCode, String responseBody) at Docker.DotNet.DockerClient.HandleIfErrorResponseAsync(HttpStatusCode statusCode, HttpResponseMessage response, IEnumerable`1 handlers) at Docker.DotNet.DockerClient.MakeRequestAsync(IEnumerable`1 errorHandlers, HttpMethod method, String path, IQueryString queryString, IRequestContent body, IDictionary`2 headers, TimeSpan timeout, CancellationToken token) at Docker.DotNet.ContainerOperations.InspectContainerAsync(String id, CancellationToken cancellationToken) at DotNet.Testcontainers.Clients.DockerContainerOperations.ByIdAsync(String id, CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Clients/DockerContainerOperations.cs:line 36 at DotNet.Testcontainers.Containers.DockerContainer.CheckReadinessAsync(WaitStrategy waitStrategy, CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/DockerContainer.cs:line 531 at DotNet.Testcontainers.Configurations.WaitStrategy.<>c__DisplayClass24_0.<<WaitUntilAsync>g__UntilAsync|0>d.MoveNext() in ~/testcontainers-dotnet/src/Testcontainers/Configurations/WaitStrategies/WaitStrategy.cs:line 184 --- End of stack trace from previous location --- at DotNet.Testcontainers.Configurations.WaitStrategy.WaitUntilAsync(Func`1 wait, TimeSpan interval, TimeSpan timeout, Int32 retries, CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Configurations/WaitStrategies/WaitStrategy.cs:line 213 at DotNet.Testcontainers.Containers.DockerContainer.CheckReadinessAsync(IEnumerable`1 waitStrategies, CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/DockerContainer.cs:line 552 at DotNet.Testcontainers.Containers.DockerContainer.UnsafeStartAsync(CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/DockerContainer.cs:line 479 at DotNet.Testcontainers.Containers.DockerContainer.StartAsync(CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/DockerContainer.cs:line 282 at DotNet.Testcontainers.Containers.ResourceReaper.GetAndStartNewAsync(Guid sessionId, IDockerEndpointAuthenticationConfiguration dockerEndpointAuthConfig, IImage resourceReaperImage, IMount dockerSocket, ILogger logger, Boolean requiresPrivilegedMode, TimeSpan initTimeout, CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/ResourceReaper.cs:line 219 at DotNet.Testcontainers.Containers.ResourceReaper.GetAndStartNewAsync(Guid sessionId, IDockerEndpointAuthenticationConfiguration dockerEndpointAuthConfig, IImage resourceReaperImage, IMount dockerSocket, ILogger logger, Boolean requiresPrivilegedMode, TimeSpan initTimeout, CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/ResourceReaper.cs:line 243 at DotNet.Testcontainers.Containers.ResourceReaper.GetAndStartDefaultAsync(IDockerEndpointAuthenticationConfiguration dockerEndpointAuthConfig, ILogger logger, Boolean isWindowsEngineEnabled, CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/ResourceReaper.cs:line 135 at DotNet.Testcontainers.Clients.TestcontainersClient.RunAsync(IContainerConfiguration configuration, CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Clients/TestcontainersClient.cs:line 294 at DotNet.Testcontainers.Containers.DockerContainer.UnsafeCreateAsync(CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/DockerContainer.cs:line 415 at DotNet.Testcontainers.Containers.DockerContainer.StartAsync(CancellationToken ct) in ~/testcontainers-dotnet/src/Testcontainers/Containers/DockerContainer.cs:line 27 ``` By the way, the solution is `export TESTCONTAINERS_RYUK_CONTAINER_PRIVILEGED=true`, thank you [Stack Overflow](https://stackoverflow.com/questions/71549856/testcontainers-with-podman-in-java-tests/75110548#75110548)!
472741f
to
f40eebf
Compare
catch (DockerApiException) | ||
{ | ||
_container = null; | ||
} |
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.
👍 I think this should be:
catch (DockerApiException) | |
{ | |
_container = null; | |
} | |
catch (DockerApiException) | |
{ | |
_container = new ContainerInspectResponse(); | |
} |
We can apply the same changes to the image, network and volume operation classes. I can do it in a follow-up PR.
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.
I think setting to null
is correct (it was the previous behavior) because the Exists
method just below checks _container != null
.
We can apply the same changes to the image, network and volume operation classes. I can do it in a follow-up PR.
I tried this but it needs some more adaptation too: https://github.com/testcontainers/testcontainers-dotnet/runs/29692350906
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.
I remember it was like that before: 6e31cf3. IIRC it was/is not supposed to be null (and with your change we do not set it to null again). Even in UnsafeDeleteAsync(CancellationToken)
we set it to an instance.
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.
I tried this but it needs some more adaptation too: https://github.com/testcontainers/testcontainers-dotnet/runs/29692350906
This is based on how we determine if an image already exists and whether we need to pull it. I updated the PR, LMKWYT. I will merge it if you are good with the changes
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.
Thanks, PR looks good 👍.
QQ: Is this the default and recommended configuration for setting up a Podman container runtime?
export DOCKER_HOST=unix://${HOME}/.local/share/containers/podman/machine/podman.sock
export TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE=/run/user/501/podman/podman.sock
export TESTCONTAINERS_RYUK_CONTAINER_PRIVILEGED=true
I can then add this to the documentation.
Yes, that's what worked for me on macOS with podman 5.2.2. |
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.
Thanks 🙌
What does this PR do?
This pull request fixes a potential
NullReferenceException
.Why is it important?
NullReferenceException
are programming errors, a better exception should always be surfaced.Related issues
How to test this PR
While experimenting with Podman I tried to run tests with the following settings:
This lead to a
NullReferenceException
The problem with this configuration is that the reaper container doesn't start but this is undiagnosable because of the swallowed exception. So instead of swallowing the exception in
ByIdAsync
theDockerApiException
is now caught insideExistsWithIdAsync
instead.Although not obvious, the exception after this commit is better and can put you on the right track to understand that the issue is about the reaper container not starting properly.
By the way, the solution is
export TESTCONTAINERS_RYUK_CONTAINER_PRIVILEGED=true
, thank you Stack Overflow!