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

Amend container shutdown to catch and log for all Exception classes #1663

Merged
merged 4 commits into from Aug 10, 2019

Conversation

rnorth
Copy link
Member

@rnorth rnorth commented Jul 24, 2019

Replacement for #1420 to fix incomplete error handling
Refs #1379

We found that unfortunately #1420 had some issues:

  • It would rethrow an exception in some cases, making tests fail unnecessarily
  • the code which parsed ConflictException messages didn't actually correctly detect the error message in the exception.
  • in addition, the exception types being caught were actually thrown off by the use of a Proxy

We decided to simplify and just catch Exception, since we don't actually care about what might have caused container deletion to fail.

It is causing exceptions like these, failing tests:

Gradle Test Executor 1 > org.testcontainers.junit.GenericContainerRuleTest > sharedMemorySetTest FAILED
    java.lang.reflect.UndeclaredThrowableException
        at com.sun.proxy.$Proxy35.exec(Unknown Source)
        at org.testcontainers.utility.ResourceReaper.stopContainer(ResourceReaper.java:233)
        at org.testcontainers.utility.ResourceReaper.stopAndRemoveContainer(ResourceReaper.java:212)
        at org.testcontainers.containers.GenericContainer.stop(GenericContainer.java:396)
        at org.testcontainers.lifecycle.Startable.close(Startable.java:18)
        at org.testcontainers.junit.GenericContainerRuleTest.sharedMemorySetTest(GenericContainerRuleTest.java:413)
        Caused by:
        java.lang.reflect.InvocationTargetException
            at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
            at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
            at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
            at java.lang.reflect.Method.invoke(Method.java:498)
            at org.testcontainers.dockerclient.AuditLoggingDockerClient.lambda$wrappedCommand$14(AuditLoggingDockerClient.java:98)
            ... 6 more
            Caused by:
            com.github.dockerjava.api.exception.ConflictException: {"message":"Cannot kill container: 6f87edb101f9d5534a872e0c3182c789ef0d8d029f1f42311c4ece540db3a2ff: Container 6f87edb101f9d5534a872e0c3182c789ef0d8d029f1f42311c4ece540db3a2ff is not running"}
                at org.testcontainers.dockerclient.transport.okhttp.OkHttpInvocationBuilder.execute(OkHttpInvocationBuilder.java:274)
                at org.testcontainers.dockerclient.transport.okhttp.OkHttpInvocationBuilder.execute(OkHttpInvocationBuilder.java:254)
                at org.testcontainers.dockerclient.transport.okhttp.OkHttpInvocationBuilder.post(OkHttpInvocationBuilder.java:115)
                at com.github.dockerjava.core.exec.KillContainerCmdExec.execute(KillContainerCmdExec.java:30)
                at com.github.dockerjava.core.exec.KillContainerCmdExec.execute(KillContainerCmdExec.java:11)
                at com.github.dockerjava.core.exec.AbstrSyncDockerCmdExec.exec(AbstrSyncDockerCmdExec.java:21)
                at com.github.dockerjava.core.command.AbstrDockerCmd.exec(AbstrDockerCmd.java:35)
                at com.github.dockerjava.core.command.KillContainerCmdImpl.exec(KillContainerCmdImpl.java:50)
                ... 11 more

Replacement for #1420 to fix incomplete error handling
Refs #1379
@rnorth
Copy link
Member Author

rnorth commented Jul 25, 2019

@bsideup I've just tweaked the exception logging a little - I realised that if we're logging an exception message, then it makes far more sense to log the root cause exception's message.

@rnorth rnorth added this to the next milestone Aug 10, 2019
@rnorth rnorth merged commit 4d1ba9e into master Aug 10, 2019
@rnorth rnorth deleted the prevent-teardown-rethrow branch August 10, 2019 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants