On 3/9/21 7:08 PM, Junio C Hamano wrote:
"Jeff Hostetler via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
+/*
+ * This value was chosen at random.
+ */
+#define WAIT_STEP_MS (50)
... and never used. Is this supposed to be used as the hardcoded
value 50 below ...
Let me fix that before you move this to "next".
Looks like I just missed that one.
+
+/*
+ * Try to connect to the server. If the server is just starting up or
+ * is very busy, we may not get a connection the first time.
+ */
+static enum ipc_active_state connect_to_server(
+ const char *path,
+ int timeout_ms,
+ const struct ipc_client_connect_options *options,
+ int *pfd)
+{
+ int wait_ms = 50;
... here?
+ int k;
+
+ *pfd = -1;
+
+ for (k = 0; k < timeout_ms; k += wait_ms) {
+ int fd = unix_stream_connect(path, options->uds_disallow_chdir);
+
+ if (fd != -1) {
+ *pfd = fd;
+ return IPC_STATE__LISTENING;
+ }
+
+ if (errno == ENOENT) {
+ if (!options->wait_if_not_found)
+ return IPC_STATE__PATH_NOT_FOUND;
+
+ goto sleep_and_try_again;
+ }
+ ...
+ return IPC_STATE__OTHER_ERROR;
+
+ sleep_and_try_again:
+ sleep_millisec(wait_ms);
Or, since there is nothing like exponential back-off implemented
here which may want to modify wait_ms variable, perhaps use the
constant directly here and where k is incremented?
+/*
+ * A randomly chosen timeout value.
+ */
+#define MY_CONNECTION_TIMEOUT_MS (1000)
Even if it may have been "randomly chosen", there should be some
criteria to judge if the value is sensible, right? IOW, I have a
suspicion that I would regret if I randomly chose 5 (or 3600000)
instead of 1000. How would we figure that 1000 acceptable but not
5?
Perhaps explain that criterion here, e.g. "... value that ought to
be long enough to establish connection locally as long as the box is
not loaded unusably heavily" or something?
Will do. Thanks!
Jeff
