( )⚙ D1467 remotefilelog: implement threaded _getfiles

This is an archive of the discontinued Mercurial Phabricator instance.

remotefilelog: implement threaded _getfiles
ClosedPublic

Authored by ikostia on Nov 20 2017, 5:47 PM.
Tags
None
Subscribers
None

Details

Reviewers
durham
Group Reviewers
Restricted Project
Commits
rFBHGXbdfae9b06013: remotefilelog: implement threaded _getfiles
Summary

A better way to avoid deadlocks and not sacrifice performance on _getfiles
call.

Test Plan
  • build, pull and update on Windows
  • build, pull and update on Linux
  • do not observe it hanging

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ikostia created this revision.Nov 20 2017, 5:47 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 20 2017, 5:47 PM
ikostia updated this revision to Diff 3684.Nov 20 2017, 6:26 PM

hiding behind a config

durham requested changes to this revision.Nov 21 2017, 10:33 AM
durham added inline comments.
remotefilelog/fileserverclient.py
263

We probably want twriterthread.daemon = True, that way if the main thread throws an exception, this thread doesn't keep the process alive forever.

269

If the background thread throws an exception for some reason, this receivemissing could be stuck forever right? Since we're trying to read the response to a request that was never sent. I think that's one of the big problems the 26bca55ffbc solution was trying to solve.

This revision now requires changes to proceed.Nov 21 2017, 10:33 AM
durham added inline comments.Nov 21 2017, 10:34 AM
remotefilelog/fileserverclient.py
396

Given that this approach caused deadlocks almost immediately last time we tried it, maybe we do non-threading by default for now, and turn it on for our team and windows users for a while before we ship it more broadly.

ikostia updated this revision to Diff 3781.Nov 22 2017, 5:09 PM

make optimistic by default

durham accepted this revision.EditedNov 22 2017, 5:14 PM

We should fix the exception handling before rolling to Linux, since there's definitely infinite hang potential

This revision is now accepted and ready to land.Nov 22 2017, 5:14 PM
This revision was automatically updated to reflect the committed changes.