From: Jeff Hostetler <git@jeffhostetler.com>
Date: Mon, 15 Mar 2021 19:56:49 GMT
Subject: Re: [PATCH v5 11/12] simple-ipc: add Unix domain socket implementation
Message-ID: <9e6a041c-0581-73ab-0139-8c6d507075b3@jeffhostetler.com>
In-Reply-To: <xmqq7dmfzx8q.fsf@gitster.c.googlers.com>



On 3/9/21 7:08 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> 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