Patchwork chg: make timeout adjustable

login
register
mail settings
Submitter Jun Wu
Date June 13, 2016, 8:41 p.m.
Message ID <e7be85fe1486decc6f0e.1465850486@x1c>
Download mbox | patch
Permalink /patch/15478/
State Accepted
Headers show

Comments

Jun Wu - June 13, 2016, 8:41 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1465849814 -3600
#      Mon Jun 13 21:30:14 2016 +0100
# Node ID e7be85fe1486decc6f0e7e46df7cbbb30a249668
# Parent  c27dc3c31222c7f74331221a3d25566146feecac
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r e7be85fe1486
chg: make timeout adjustable

Before this patch, chg will give up when it cannot connect to the new server
within 10 seconds. If the host has high load during that time, 10 seconds
is not enough.

This patch makes it adjustable using the CHGTIMEOUT environment variable.
Yuya Nishihara - June 14, 2016, 2:20 p.m.
On Mon, 13 Jun 2016 21:41:26 +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1465849814 -3600
> #      Mon Jun 13 21:30:14 2016 +0100
> # Node ID e7be85fe1486decc6f0e7e46df7cbbb30a249668
> # Parent  c27dc3c31222c7f74331221a3d25566146feecac
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r e7be85fe1486
> chg: make timeout adjustable

Looks fine, queued. Thanks.

> Before this patch, chg will give up when it cannot connect to the new server
> within 10 seconds. If the host has high load during that time, 10 seconds
> is not enough.
> 
> This patch makes it adjustable using the CHGTIMEOUT environment variable.

Is it mainly for automated tests?

> +	unsigned int timeoutsec = 10;  /* default: 10 seconds */
> +	const char *timeoutenv = getenv("CHGTIMEOUT");
> +	if (timeoutenv)
> +		sscanf(timeoutenv, "%u", &timeoutsec);

I was slightly afraid of this, but it seems fine to expect the original
timeoutsec is kept unmodified on failure.

http://stackoverflow.com/questions/25715204/
Jun Wu - June 14, 2016, 3:33 p.m.
Excerpts from Yuya Nishihara's message of 2016-06-14 23:20:49 +0900:
> Is it mainly for automated tests?

No. We got several user complaints about the timeout error and I found their
machines were overloaded. I plan to set CHGTIMEOUT=0 internally to avoid
future complaints. It makes sense since it's guaranteed that the hg
executable shipped is legit so I can count on "if hg serve runs, it must
create the socket file".

> I was slightly afraid of this, but it seems fine to expect the original
> timeoutsec is kept unmodified on failure.
> 
> http://stackoverflow.com/questions/25715204/ 

I used the pattern a lot before therefor didn't think twice about it.
Fortunately the standards are sane in this case.
Yuya Nishihara - June 15, 2016, 1:27 p.m.
On Tue, 14 Jun 2016 16:33:29 +0100, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2016-06-14 23:20:49 +0900:
> > Is it mainly for automated tests?  
> 
> No. We got several user complaints about the timeout error and I found their
> machines were overloaded. I plan to set CHGTIMEOUT=0 internally to avoid
> future complaints. It makes sense since it's guaranteed that the hg
> executable shipped is legit so I can count on "if hg serve runs, it must
> create the socket file".

Sounds like the default is too short. You can work around the problem, but
we'd better change the default to 30 or 60 sec since the problem happened in
production environment.
Jun Wu - June 15, 2016, 1:47 p.m.
Excerpts from Yuya Nishihara's message of 2016-06-15 22:27:40 +0900:
> Sounds like the default is too short. You can work around the problem, but
> we'd better change the default to 30 or 60 sec since the problem happened
> in production environment.

That's a good idea. 60 sounds like a good default. I will submit a patch
after fixing hgsvn test failures.

Patch

diff --git a/contrib/chg/README b/contrib/chg/README
--- a/contrib/chg/README
+++ b/contrib/chg/README
@@ -28,3 +28,5 @@ 
 
  * CHGDEBUG enables debug messages.
  * CHGSOCKNAME specifies the socket path of the background cmdserver.
+ * CHGTIMEOUT specifies how many seconds chg will wait before giving up
+   connecting to a cmdserver. If it is 0, chg will wait forever. Default: 10
diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -249,7 +249,13 @@ 
 	int pst = 0;
 
 	debugmsg("try connect to %s repeatedly", opts->sockname);
-	for (unsigned int i = 0; i < 10 * 100; i++) {
+
+	unsigned int timeoutsec = 10;  /* default: 10 seconds */
+	const char *timeoutenv = getenv("CHGTIMEOUT");
+	if (timeoutenv)
+		sscanf(timeoutenv, "%u", &timeoutsec);
+
+	for (unsigned int i = 0; !timeoutsec || i < timeoutsec * 100; i++) {
 		hgclient_t *hgc = hgc_open(opts->sockname);
 		if (hgc)
 			return hgc;