| 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
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/
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.
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.
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;