#446 make default docker client thread safe

[unicolet]

Fixes #446

The problem was that DEFAULT_CONFIG in DefaultDockerClient.java was not thread safe. The rest is just Intellij optimizations and a test to verify the fix.

If you merge this PLEASE release a new version of maven-docker-client with the fix, which is the only reason for this PR to exist in the first place.

[mattnworb]

Thanks for the contribution. Looks like the build fails because of the whitespace changes, particularly those in the license header. I'm afraid the plugin used for that is sensitive to whitespace changes and wants a trailing space on otherwise empty lines.

Can you fix those?

Two other questions:

1. do you have any reference to ClientConfig not being thread-safe, or any idea where in it's implementation the unsafeness is coming from?
2. any idea if this is related to Waiting threads when using a client concurrently #727?

[unicolet]

@mattnworb thank for reaching out, sure I'll fix them. I was not sure what the problem was 😄

1. do you have any reference to ClientConfig not being thread-safe, or any idea where in it's implementation the unsafeness is coming from?

we use the maven plugin and it only breaks once you reach a certain number of parallel tasks. The problem is not with ClientConfig itself which, AFAIK, might be perfectly thread safe, but with the fact that, being static, it is shared across all instances of DefaultDockerClient without locking. So it might happen that two clients share the same thread pool. When one finishes it shuts down the pool and the other client gets the exception when it attempts another operation. Try running the test with and without static :-)

2. any idea if this is related to Waiting threads when using a client concurrently #727?

sorry, no

[mattnworb]

The problem is not with ClientConfig itself which, AFAIK, might be perfectly thread safe, but with the fact that, being static, it is shared across all instances of DefaultDockerClient without locking. So it might happen that two clients share the same thread pool.

Is the thread pool owned by the Client or the ClientConfig?

I've found some conflicting information on this:

• http://stackoverflow.com/questions/14305655/jersey-client-services-closing-resources
• JERSEY-3306 (which is sadly only available in the Google cache since java.net was closed down)

[unicolet]

@mattnworb sorry I am in a bit of a rush, I wrote thread pool when I meant connection pool.

There is a small window of opportunity between here and here when two instances might share the same connection pool.

I have made some research a couple of weeks ago and then paused it to take care of other stuff so I'm not 100% fresh :-) My research was that all other parts are fine and/or were fixed in earlier releases than those this project uses currently.

[mattnworb]

@unicolet good catch - .property(ApacheClientProperties.CONNECTION_MANAGER, cm) is mutating the original (shared) CLIENT_CONFIG. Seems like maybe someone may have assumed that those methods were returning new instances and that the config was immutable.

Thanks for clarifying btw - I just wanted to be extra sure that this is actually the problem before we claim to have fixed #446

[unicolet]

@mattnworb I need this more than you think 😛 Please, please, please with 🍒 on top make sure this makes it into maven plugin. Could you look at why the tests fail? I will fix it right away if you could give me a hint :-)

[mattnworb]

the two test failures look transient/random to me, but I kicked them off again just to double-check

[unicolet]

@mattnworb still failing, I'll take a look tomorrow. Cheers :-)

[mattnworb]

after re-running the tests in travis a bunch of times I am convinced the failures are due to flaky tests. Thanks again for the contribution!

[unicolet]

@mattnworb thanks, please port this to https://github.com/spotify/docker-maven-plugin ❤️