Repository navigation
Preserve the order of requests to Docker daemon using DefaultInvocationBuilder (#1492) - #1494
Conversation
Sorry, something went wrong.
| thread.start(); | ||
|
|
||
| try { | ||
| onStart.await(); |
There was a problem hiding this comment.
callback's onStart should be used to detect started operation. Blocking here is an unexpected behaviour change
Sorry, something went wrong.
There was a problem hiding this comment.
also see awaitStarted on ResultCallbackTemplate
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry for the possible confusion: onStart is CountDownLatch here actually. I will rename it.
Sorry, something went wrong.
There was a problem hiding this comment.
my point is that DefaultInvocationBuilder should not be changed. Instead, the callee should use awaitStarted() or similar means
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you, I got it now. I thought there will be inconsistency with other post and get methods of DefaultInvocationBuilder which do not accept ResultCallback as a parameter. The requests in these cases are executed in the same thread and awaitStarted() is not required. But this is the expected behavior?
Sorry, something went wrong.
There was a problem hiding this comment.
Those that have no ResultCallback as a parameter are blocking methods, while ResultCallback-based ones are kinda asynchronous, hence no blocking behaviour
Sorry, something went wrong.
bsideup
left a comment
There was a problem hiding this comment.
👍
Sorry, something went wrong.
`attachContainerStdinUnsupported()` test requires neither starting the container nor waiting on `awaitStarted()` of attach's callback.
Restore starting the container in the test.
The problem is that using awaitStarted() on attach's callback on a non-running container leads to the infinite wait for Jersey transport implementation.
This happens when SelectiveLoggingFilter is used in DefaultInvocationBuilder where it is created with printEntity = true parameter. In this case the implementation of ClientResponseFilter.filter(...) method in the base LoggingFilter class blocks at responseContext.hasEntity() until the data is available in the response. This happens because hasEntity() reads a single byte to check whether there is any data available in the stream. In the case of attach command this method blocks until there is data in stdout/stderr streams. Having data from the streams requires container running, which we cannot do safely without awaitStarted() because attach command is performed in a separate thread and we need to run start only after attach request is executed not to loose the data from stdout/stderr streams.