Don't use WithBlock() on dist gRPC connection#986
Conversation
When using WithBlock() on the dialer, the connection timeout must fully expire before any status is provided to the user about whether they can even connect to the socket. For example, if the containerd socket is root-owned and the user tries `dist images ls` without `sudo`, the default is 30 sec. of "hang" before the command returns. Signed-off-by: Phil Estes <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #986 +/- ##
=======================================
Coverage 59.05% 59.05%
=======================================
Files 5 5
Lines 779 779
=======================================
Hits 460 460
Misses 205 205
Partials 114 114Continue to review full report at Codecov.
|
|
LGTM |
|
In this case, the timeout doesn't have any effect according to the doc: // WithTimeout returns a DialOption that configures a timeout for dialing a ClientConn
// initially. This is valid if and only if WithBlock() is present. |
|
@mlaventure hmm, does seem to be the case. That means all the |
|
LGTM I agree with @estesp, no point in having |
|
wouldn't that mean that if I do:
There's a chance that it fails directly? because the server just wasn't ready in time? |
|
Reading through this code for the first time, so not fully sure, but looks like: is used even without the EDIT: Specifically here if I'm following the non-blocking path correctly. |
|
LGTM Merging this one, for now. Let's have another PR to handle the timeout. |
6 participants
When using WithBlock() on the dialer, the connection timeout must fully
expire before any status is provided to the user about whether they can
even connect to the socket. For example, if the containerd socket is
root-owned and the user tries
dist images lswithoutsudo, thedefault is 30 sec. of "hang" before the command returns.
Signed-off-by: Phil Estes [email protected]
// cc: @stevvooe per our discussion