gclient: kill git fetch operation that hangs.

gclient: kill git fetch operation that hangs.

This provides env variable GCLIENT_KILL_GIT_FETCH_AFTER
that kills git fetch if it produces no output for that
many seconds.

Note that this is not final patch, but an experiment.
See http://crbug.com/635641#c24 for the deployment plan.

BUG=635641
R=hinoka@chromium.org

Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/f8757b7e02226594655230ccbeae3543c7dc49c6

[tandrii(chromium), 2016-08-12 18:16:04]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 18:16:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 18:19:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 18:19:01]

Dry run: This issue passed the CQ dry run.

[tandrii(chromium), 2016-08-12 18:58:17]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 18:58:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 19:01:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 19:01:14]

Dry run: This issue passed the CQ dry run.

[tandrii(chromium), 2016-08-16 16:18:50]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 16:18:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 16:24:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 16:24:46]

Dry run: Try jobs failed on following builders:
  Depot Tools Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30acf7bcea558a10)

[tandrii(chromium), 2016-08-16 16:33:14]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 16:33:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 16:36:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 16:36:27]

Dry run: This issue passed the CQ dry run.

[tandrii(chromium), 2016-08-16 16:37:28]

tandrii@chromium.org changed reviewers:
+ hinoka@chromium.org

[tandrii(chromium), 2016-08-16 16:37:28]

PTL at my approach first, then if you like it, at tests.

[Ryan Tseng, 2016-08-16 23:59:00]

hinoka@google.com changed reviewers:
+ hinoka@google.com

[Ryan Tseng, 2016-08-16 23:59:01]

I'm wary of starting and stopping a thread everytime a line is printed.  Threads
are pretty heavyweight in python i think and this would lead to some funky
behavior.  Consider just launching one thread per process (the nag thread) and
use some form of message passing to poke it.

I'm also wary of parsing git text, since those could change from under us
without warning.  In fact I think it might be fine not to check for delta
compression mode since our bots are actually not supposed to spend that much
time doing delta compression because we turned off autogc.

otherwise approach is good with me

[tandrii(chromium), 2016-08-17 11:16:30]

On 2016/08/16 23:59:01, Ryan Tseng wrote:
> I'm wary of starting and stopping a thread everytime a line is printed. 
Threads
> are pretty heavyweight in python i think and this would lead to some funky
> behavior.  Consider just launching one thread per process (the nag thread) and
> use some form of message passing to poke it.
That's true. I'll redo. But doesn't it apply to PsPrinter from bot_update as
well? My code is based on it.

> I'm also wary of parsing git text, since those could change from under us
> without warning.  In fact I think it might be fine not to check for delta
> compression mode since our bots are actually not supposed to spend that much
> time doing delta compression because we turned off autogc.
I'm parsing text to *avoid* killing process that has finished receiving stuff
from remote.
Worst case if git changes output is that my code kills git fetch during
unpacking or delta processing, which as you argue above is OK.
Thus, the question is whether there is any value in having that text parsing.
WdYT?

> otherwise approach is good with me
OK.

[tandrii(chromium), 2016-08-17 11:57:00]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 11:57:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tandrii(chromium), 2016-08-17 11:58:06]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 11:58:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tandrii(chromium), 2016-08-17 11:59:37]

See PS#6 - no more thread creation on every char.

[commit-bot: I haz the power, 2016-08-17 12:00:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 12:00:19]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2016-08-18 18:32:45]

Given that you'd otherwise be able to halve this CL (by removing the
nag_interval/fn abstraction and implementing a kill_timeout instead), I don't
think there's value to adding text parsing.  This is my argument:
* Text parsing is flaky.  It'll work today but who knows if it'll work next year
with git 2.larger_number.
* An initial git cache checkout will never need to delta decompress because the
bundle we provide is already uncompressed.  We also re-bootstrap on every 50
fetch to avoid running GC.  Hence, 95+% of the runtime of git fetch should be in
the git-remote-https call.
* If we do have delta decompressions that take upwards of 20 minutes, that's a
bug and we should surface it and fix it.

Anyways, it's your call.  I'll lgtm (% TODO) this because I think it's fine, but
it could be shorter.

[tandrii(chromium), 2016-08-18 18:56:22]

On 2016/08/18 18:32:45, Ryan Tseng wrote:
> Given that you'd otherwise be able to halve this CL (by removing the
> nag_interval/fn abstraction and implementing a kill_timeout instead), I don't
> think there's value to adding text parsing.  This is my argument:
> * Text parsing is flaky.  It'll work today but who knows if it'll work next
year
> with git 2.larger_number.
> * An initial git cache checkout will never need to delta decompress because
the
> bundle we provide is already uncompressed.  We also re-bootstrap on every 50
> fetch to avoid running GC.  Hence, 95+% of the runtime of git fetch should be
in
> the git-remote-https call.
> * If we do have delta decompressions that take upwards of 20 minutes, that's a
> bug and we should surface it and fix it.
> 
> Anyways, it's your call.  I'll lgtm (% TODO) this because I think it's fine,
but
> it could be shorter.

I 100% buy halving this CL. Will upload new version soon.

[tandrii(chromium), 2016-08-24 14:04:49]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 14:04:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 14:10:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 14:10:22]

Dry run: Try jobs failed on following builders:
  Depot Tools Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30d5afd8f5c40110)

[tandrii(chromium), 2016-08-24 14:46:18]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 14:46:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tandrii(chromium), 2016-08-24 14:46:36]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 14:46:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 14:54:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 14:54:14]

Dry run: Try jobs failed on following builders:
  Depot Tools Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30d5d9d8a1ea6b10)

[tandrii(chromium), 2016-08-24 15:06:10]

PTAL

not really halved, but removed unnecessary stuff yet kept test thorough.

Deployment plan: see
https://bugs.chromium.org/p/chromium/issues/detail?id=635641#c24

https://codereview.chromium.org/2241843002/diff/140001/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/2241843002/diff/140001/gclient_scm.py#newcode...
gclient_scm.py:1202: kill_timeout =
float(os.getenv('GCLIENT_KILL_GIT_FETCH_AFTER', 0))
fyi: i intend to remove this once I actually deploy this to all bots with
https://codereview.chromium.org/2273043002

[tandrii(chromium), 2016-08-24 15:12:25]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 15:12:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 15:18:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 15:18:06]

Dry run: Try jobs failed on following builders:
  Depot Tools Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30d5edda37e59010)

[tandrii(chromium), 2016-08-24 15:19:11]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 15:19:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tandrii(chromium), 2016-08-24 15:21:57]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 15:22:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 15:24:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 15:24:56]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2016-08-25 00:06:21]

lgtm! This is easier to follow.

remember to update description

[tandrii(chromium), 2016-08-25 10:54:59]

Description was changed from

==========
gclient: kill git fetch operation that hangs.

Prototype.

BUG=
==========

to

==========
gclient: kill git fetch operation that hangs.

This provides env variable GCLIENT_KILL_GIT_FETCH_AFTER
that kills git fetch if it produces no output for that
many seconds.

Note that this is not final patch, but an experiment.
See http://crbug.com/635641#c24 for the deployment plan.

BUG=635641
R=hinoka@chromium.org
==========

[tandrii(chromium), 2016-08-25 10:55:21]

The CQ bit was checked by tandrii@chromium.org

[tandrii(chromium), 2016-08-25 10:55:22]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@google.com
Link to the patchset: https://codereview.chromium.org/2241843002/#ps220001
(title: "Nits.")

[tandrii(chromium), 2016-08-25 10:55:28]

On 2016/08/25 00:06:21, Ryan Tseng wrote:
> lgtm! This is easier to follow.
> 
> remember to update description
Good point. done!

[commit-bot: I haz the power, 2016-08-25 10:55:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 11:02:25]

Description was changed from

==========
gclient: kill git fetch operation that hangs.

This provides env variable GCLIENT_KILL_GIT_FETCH_AFTER
that kills git fetch if it produces no output for that
many seconds.

Note that this is not final patch, but an experiment.
See http://crbug.com/635641#c24 for the deployment plan.

BUG=635641
R=hinoka@chromium.org
==========

to

==========
gclient: kill git fetch operation that hangs.

This provides env variable GCLIENT_KILL_GIT_FETCH_AFTER
that kills git fetch if it produces no output for that
many seconds.

Note that this is not final patch, but an experiment.
See http://crbug.com/635641#c24 for the deployment plan.

BUG=635641
R=hinoka@chromium.org

Committed:
https://chromium.googlesource.com/chromium/tools/depot_tools/+/f8757b7e022265...
==========

[commit-bot: I haz the power, 2016-08-25 11:02:26]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/tools/depot_tools/+/f8757b7e022265...

[tandrii(chromium), 2016-10-11 12:16:27]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2410853002/ by tandrii@chromium.org.

The reason for reverting is: Didn't help..