( )⚙ D1436 remotefilelog: make subprocess pipei buffer size configurable on Windows

This is an archive of the discontinued Mercurial Phabricator instance.

remotefilelog: make subprocess pipei buffer size configurable on Windows
ClosedPublic

Authored by ikostia on Nov 16 2017, 12:58 PM.
Tags
None
Subscribers

Details

Summary

Here's the way we communicate between hg binary and an ssh binary in fileserverclient:

while not done:
  write STEP lines to PIPEI
  read all responses from PIPEO

where PIPEI and PIPEO are communication pipes between hg and ssh, and STEP
is a configurable number of requests for file data (each looking like filename:hash).

In the past STEP used to be always 10000 and it just worked for us, because the default
Linux pipe capacity (65536, I call it the pipe buffer size) was always enough.
However, it is pretty obvious that it is possible for this thing to deadlock
if the total size of STEP lines is bigger than the buffer size. This is what
happened on Windows, where default buffer size is 4096 bytes, so we made STEP
configurable and set it to 100 on Windows, which proved to work.
A better solution is to make PIPEI have a larger buffer, which is possible if
we create the pipe manually, instead of asking subprocess to do so for us.

Later we can also get rid of the STEP altogether by just counting how many bites
we've written in each loop iteration.

Test Plan
  • does not seem to break any additional tests on Linux
  • remove hgcache, hg up master --config remotefilelog.getfilesstep=10000 does not hang, it used to previously

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Branch
default
Lint
Lint OK
Unit
Unit Tests OK

Event Timeline

ikostia created this revision.Nov 16 2017, 12:58 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 16 2017, 12:58 PM
ikostia planned changes to this revision.Nov 16 2017, 1:01 PM
ikostia updated this revision to Diff 3570.Nov 16 2017, 1:49 PM

hide behind a config knob

ikostia planned changes to this revision.Nov 16 2017, 1:49 PM
ikostia edited the test plan for this revision. (Show Details)Nov 17 2017, 12:14 PM
ikostia requested review of this revision.

Tested.

ikostia planned changes to this revision.Nov 17 2017, 2:02 PM
ikostia abandoned this revision.Nov 17 2017, 2:06 PM

This is entirely wrong.

ikostia updated this revision to Diff 3629.Nov 19 2017, 2:58 PM

rewrite

ikostia retitled this revision from remotefilelog: set subprocess pipei buffer size to 65536 on Windows to remotefilelog: make subprocess pipei buffer size configurable on Windows.Nov 19 2017, 4:03 PM
durham accepted this revision.Nov 20 2017, 1:29 PM
durham added a subscriber: durham.

I'm not super familiar with Windows, so kinda yolo'ing here.

hgext3rd/tweakdefaults.py
275

Should we just default to a large size for windows? I know other companies have hit this as well.

1060

Did you get this magic from some place? Maybe include a link to it for future readers to refer to?

1064

Usually we set close_fds to true, so the child process doesn't keep any of the parent process files alive. Any reason to set it to false here?

This revision is now accepted and ready to land.Nov 20 2017, 1:29 PM
This revision was automatically updated to reflect the committed changes.