Summary:
Overall plan to modify Future<T>::then to be r-value qualified and use Future<T>::thenTry or Future<T>::thenValue.
The goal is to disambiguate folly::Future and to improve type and lifetime safety of Future and its methods.
Codemod:
* future<T>.then(callable with operator()(not-a-try)) to future<T>.thenValue(callable with operator()(not-a-try)).
* future<T>.then(callable with operator()()) to future<T>.thenValue(callable with operator()(auto&&)).
* future<T>.then(callable with operator()(auto)) to future<T>.thenValue(callable with operator()(auto)).
Reviewed By: yfeldblum
Differential Revision: D9512177
fbshipit-source-id: daa3581611dcd9f32d9314bae1c5fa0f966613f3
Summary:
Update hg_import_helper.py to throw a new ResetRepoError type when it decides
that its local mercurial state is invalid and that it needs to reopen the
repository. The C++ code will catch this exception, restart the
hg_import_helper.py and retry the request. (It will only retry once for each
request.)
The previous behavior of simply closing and re-opening the repository object
in Python has been problematic, as this has resulted in resource leaks. The
hg_import_helper.py process itself ends up leaking memory when re-opening the
repository. This also appears to result in many scmmemcache helper processes
being created and not cleaned up. Presumably these are associated with the
old repository state and not cleaned up properly when we reopen the
repository.
Reviewed By: quark-zju
Differential Revision: D9510664
fbshipit-source-id: 449dfa9e2e21aabf8b3ce640749d32aa8f8e4052
Summary:
Update the HgImporter code to actually check the transaction ID field in the
response headers from the hg_import_helper script.
Reviewed By: wez
Differential Revision: D9507288
fbshipit-source-id: 0a170bd6b39426176c938a135508fa4bb928053a
Summary:
Goal: change catchErrors() so its param is an rvalue-ref (`template <typename T> ... catchErrors(Future<T>&& fut)`) instead of a forwarding ref (and, if necessary, adjust callsites that passed an lvalue-ref).
Rationale: this is part of T33085035 which is migrating Future::onError() to be rvalue-only. D9505757 codemodded `RequestData::catchErrors()` from `fut.onError()` to `std::move(fut).onError()`. However `fut` was a forwarding-ref so it can actually hold an lvalue-ref.
Timeline/urgency: this diff is a prereq to removing the lvalue-qual overload of Future::onError() (T33085035). (Technically the only part of this that is a prereq to T33085035 is to adjust callers of catchErrors() that pass lvalue Futures, but changing the catchErrors() signature is righteous since it will force a more sensible error-message on any future callers which erroneously pass an lvalue.)
Note: D9505757 landed since it didn't break anything, even though it used `std::move()` on a forwarding-ref. (The lvalue-qual and rvalue-qual overloads of Future::onError() currently have identical semantics; neither invalidates / moves-out its Future object.)
Reviewed By: yfeldblum
Differential Revision: D9510204
fbshipit-source-id: 5c1ff2f36c5d84c583268f5e952fd322ea8e9327
Summary:
Update the HgImporter code to check the return value from
`folly::writevFull()`
Reviewed By: wez
Differential Revision: D9507289
fbshipit-source-id: 00985f50ebb4717f7b97a199bf9efba6b4f5f628
Summary:
Previously the HgImporter code was not checking the return value of
`folly::readFull()`. This function does not throw exceptions: it may return
-1 on error or a partial number of bytes read. Previously the HgImporter code
did not perform any checking of the result from this function.
Reviewed By: wez
Differential Revision: D9507287
fbshipit-source-id: 80d8bdb54375eb287d1473675fffb48c4cd34c56
Summary:
These methods were only used in the test utility in one location. I plan to
clean up how the HgImporter code reads from the underlying python process, and
it will be easier if these are non-static methods.
This removes the ability from the test utility to read from a pre-computed
manifest data file. I don't think we care too much about this functionality
any more--most of our repositories are exclusively using treemanifest mode now
anyway.
Reviewed By: wez
Differential Revision: D9507290
fbshipit-source-id: 97fe3fc9d8574c97a41328ed42e7afdacb3734d1
Summary:
Add a wrapper class around HgImporter that can catch errors communicating with
the underlying `hg_import_helper.py` script. If something goes wrong
communicating with the python code we restart the import helper and retry
the operation once.
This code only retries on HgImporterError exceptions, and nothing throws these
at the moment. Therefore for now this should not result in any behavior
changes. In the future I will update our error handling to throw
HgImporterError when we fail to communicate with the underlying
hg_import_helper.py script.
Reviewed By: wez
Differential Revision: D9507291
fbshipit-source-id: 0167959df6292a577942e049576669fb6711ce76
Summary:
This is part of "the great r-valuification of folly::Future":
* This is something we should do for safety in general.
* Several of folly::Future's methods are lvalue-qualified even though they act as though they are rvalue-qualified, that is, they provide a postcondition that says, in effect, callers should act as though the method invalidated its `this` object (regardless of whether that invalidation was actual or logical).
* This violates the C++ principle to "Express ideas directly in code" (see Core Guidelines), and generally makes it more confusing for callers as well as hiding the actual semantics from tools (linters, compilers, etc.).
* This dichotomy and confusion has manifested itself by some failures around D7840699 since lvalue-qualification hides that operation's move-out semantics - leads to some use of future operations that are really not correct, but are not obviously incorrect.
* The goal of rvalueification is to make sure methods that are logically rvalue-qualified are actually rvalue-qualified, which forces callsites to acknowledge that rvalueification, e.g., `std::move(f).onError(...)` instead of `f.onError(...)`. This syntactic change in the callsites forces callers to acknowledge the method's rvalue semantics.
Codemod changes:
* expr.onError(...) ==> std::move(expr).onError(...) // if expr is not already an xvalue
* expr->onError(...) ==> std::move(*expr).onError(...)
Note: operator precedence of that last step is safe - no need to parenthesize `expr`. Reason: `->` binds more tightly than unary `*`.
Reviewed By: yfeldblum
Differential Revision: D9505757
fbshipit-source-id: de666f3a877313526d10f5d3569a1bbb2203f066
Summary:
D9494651 and D9415207 logically conflicted with each other, but did not have
conflicts at the source code level. They both landed independently which
resulted in a broken build even though CI had passed on their individual
diffs. This updates them to be compatible.
Reviewed By: wez, strager
Differential Revision: D9505360
fbshipit-source-id: 72743167d934d1d57044cda8d5a7af03346ab142
Summary:
Change HgImporter::importFileContents() to return a Blob object rather than
just the IOBuf.
This shouldn't have any behavior changes. However, it might help track down
some of the cases where we have seen files get incorrectly imported with empty
contents in some rare cases. We're not sure what is causing that, but
incorrectly using a moved-from IOBuf object would be one thing that might
produce this behavior. By changing this code to a `unique_ptr<Blob>` will
will eliminate the possibility of seeing empty buffer contents if some code
path is somehow ever able to use one of these objects after they have been
moved away from. (The code would end up with a null pointer in this case
rather than an empty buffer. If this ever does occur in practice a null
pointer would make the problem more obvious than an empty buffer.)
Reviewed By: chadaustin
Differential Revision: D9494651
fbshipit-source-id: 799cc1b1d0e2ac32ae2013dc80c84ec17cc1c491
Summary:
This updates the `hg_import_helper.py` code so that when importing files it
also includes the file length in the response, in addition to the response
contents itself. This information is redundant and exists purely to help
the receiving C++ code verify that the response is in fact completely valid.
This is implemented with a new command number, and `hg_import_helper.py` still
supports the old command number with the old behavior so that it will still
work if old edenfs daemon instances start new instances of
`hg_import_helper.py`.
Reviewed By: chadaustin
Differential Revision: D9494653
fbshipit-source-id: 34d1221b6048d6016b30fd7f606ca0b140552d80
Summary:
In D9486915 I updated the code to invoke `hg` from the local repository, but
this was invoking the telemetry wrapper, and this was still falling through
and running the system-installed `hg.real` binary rather than the from the
local repository. This fixes the code to also pass through the `HG_REAL_BIN`
and `CHG_BIN` environment variables when invoking the `hg` wrapper so that the
underling real hg code is picked up from the current repository as wel.
Reviewed By: chadaustin
Differential Revision: D9494271
fbshipit-source-id: 89c5154f501536b9d7a43d472b1204f10e5dc664
Summary: This commit makes eden ask Mononoke API Server first when it is fetching blobs.
Reviewed By: chadaustin
Differential Revision: D9415207
fbshipit-source-id: 1146a9b6295e8ddb15e4bcb098379ea75886baa5
Summary:
The desired end state for how next inode number gets passed across
takeover. This should not land as is - the prior diffs need to land
and wait for a while until everyone transitions.
Reviewed By: simpkins
Differential Revision: D8195550
fbshipit-source-id: 2fc40f881cc1a331df95ef99f7e9e4f2e69c8643
Summary:
Prior to this diff, to read the inode metadata from an InodeBase, you
had to cast it to a FileInode or TreeInode. Add a public getMetadata
call that acquires the appropriate lock depending on whether it's a
FileInode or TreeInode.
Reviewed By: simpkins
Differential Revision: D9327180
fbshipit-source-id: 66594fb639f1ea72a0e48dcb79234e14d87ec4a2
Summary:
Update the TARGETS file for eden/fs/store/hg/test to make sure that they
explicitly specify EDEN_HG_BINARY in the environment so that the test code
will run against hg from the local repository rather than the system-installed
mercurial.
Also update testharness/HgRepo.cpp to use this environment variable when
searching for the hg binary.
Reviewed By: chadaustin, wez
Differential Revision: D9486915
fbshipit-source-id: f8f8a1acde9e8b0a032a026fa477b21c70afacbc
Summary: The error message for FLAGS_etcEdenDir should contain its value.
Reviewed By: chadaustin
Differential Revision: D9484715
fbshipit-source-id: edecf42c74830a672ab9b3cdfac357b9abdedaf8
Summary:
Fix the code in `eden_dirstate._call_match_callbacks()` to correctly match
ignored symlinks that are explicitly listed in the matcher's file list.
It looks like this was just a mistake in the original code on my part: I
intended to check both `S_ISREG()` and `S_ISLNK()` but instead incorrectly put
the check for `S_ISREG()` twice.
Reviewed By: wez
Differential Revision: D9476524
fbshipit-source-id: 67e0fa7c2fbaac97598a8e2d028c9ef0999ed88a
Summary: Modifies a few callsites in Eden that indirect through a separately allocated lambda to replace Future::then with Future::thenValue.
Reviewed By: yfeldblum
Differential Revision: D9485137
fbshipit-source-id: 84fa03a736a0faa162372e221625d82813a68620
Summary:
When we load an empty blob from the LocalStore double check with the
BackingStore to confirm that it should actually be empty.
We have seen multiple instances of files being incorrectly imported as empty.
So far this error has always been fixed by a re-import. We still haven't
tracked down the root cause, but this change should help workaround the issue
by ensuring that we double check the file contents before returning the data.
Reviewed By: chadaustin
Differential Revision: D9476522
fbshipit-source-id: 6d57cf15c42736ecbcb106a731259b77db06d8f1
Summary:
`F14NodeMap` has proven to be a safe and more performant drop-in replacement
for `std::unordered_map`. With the ability to do heterogeneous lookup and
mutation, we no longer need `StringKeyedUnorderedMap`, whose main purpose was
to avoid unnecessary string copies during lookups and inserts.
Reviewed By: nbronson
Differential Revision: D9124737
fbshipit-source-id: 81d5be1df9096ac258a3dc1523a6a0f5d6b9e862
Summary:
D9389865 changed the rel_path variable from a string to a pathlib.Path object.
However it missed a couple places where the path object needed to be converted
to bytes.
Reviewed By: chadaustin
Differential Revision: D9476525
fbshipit-source-id: 2dc69f5a0378ffbc02b2724a097604df288683c9
Summary:
Overall plan to modify Future<T>::then to be r-value qualified and use Future<T>::thenTry or Future<T>::thenValue.
The goal is to disambiguate folly::Future and to improve type and lifetime safety of Future and its methods.
Codemod:
* future<T>.then(callable with operator()(not-a-try)) to future<T>.thenValue(callable with operator()(not-a-try)).
* future<T>.then(callable with operator()()) to future<T>.thenValue(callable with operator()(auto&&)).
* future<T>.then(callable with operator()(auto)) to future<T>.thenValue(callable with operator()(auto)).
Reviewed By: chadaustin
Differential Revision: D9443286
fbshipit-source-id: be712b58b92dc7422f128713deaf6f46b29b36ce
Summary:
Update the `_get_watch_roots_for_watchman()` check so that it does not fail if
there is no `roots` entry in the output from `watchman watch-list`
Reviewed By: wez
Differential Revision: D9445138
fbshipit-source-id: e066375db3e963d08a91aa2e257d6c4fdd18da74
Summary:
Existing timer precision (milliseconds) is too coarse to
debug many operations, it will often round down to 0ms. I didn't see
any rationale for not using something more precise here.
Reviewed By: chadaustin
Differential Revision: D9437556
fbshipit-source-id: 6a79431c5a976e933ecda228aa5386c07b8fb668
Summary:
Add an `--extract-to` flag to `eden debug overlay` to ask it to copy inode
data out of the overlay. When used with a file inode this copies the file
contents. When used with a directory inode this copies the directory
structure and file contents for all materialized files under that directory.
This makes it easier to extract user's untracked and modified files out of the
overlay when recovering state after an unclean filesystem shutdown.
Reviewed By: wez
Differential Revision: D9445559
fbshipit-source-id: 9abcf82695014d79f248ba5fdb09727a49f3098a
Summary:
Allow callers to explicitly pass in the location of an overlay storage
directory, rather than requiring that this be found from a running Eden
instance.
This makes it easier to make copies of an overlay for later offline debugging.
Reviewed By: wez
Differential Revision: D9445560
fbshipit-source-id: b1f2ee0dc8bfca80dcc02c57c348f65599ab86e2
Summary:
Update all of the `eden debug` commands to use the newer `find_checkout()`
code rather than older `get_mount_path()` function.
The `find_checkout()` code makes sure that the EdenInstance actually points to
the correct edenfs instance for this checkout, and also works with checkouts
that are not currently mounted. In particular this allows
`eden debug overlay` to examine the overlay state even when the checkout is
not currently mounted.
Reviewed By: wez
Differential Revision: D9389865
fbshipit-source-id: 00578519d4805157a30c9b39abee9838925e8e76
Summary:
This adds a new `find_eden()` method to the Eden CLI code which can look up
information about the correct EdenCheckout and EdenInstance given a path.
In the future most CLI commands should switch to use this function over the
current `get_eden_instance()` and `get_mount_path()` methods. These older
APIs only work to find currently mounted checkouts, which is inadequate for
commands like `fsck` and some other debug commands that want to be able to
operate on unmounted checkouts.
This new logic is able to correctly find checkout information even for
unmounted checkouts. If the checkout is managed by an edenfs instance running
out of a non-default location it also correctly finds the state information
for Eden, rather than potentially using the wrong Eden state directory.
Reviewed By: wez
Differential Revision: D9385821
fbshipit-source-id: a6638f3c3817a595a7b7979c5cd218a2d7400f51
Summary: If the OS is Windows it will expect the in-fd and out-fd as Windows handles and convert them to CRT fds. This will not change the behavior if it's not Windows.
Reviewed By: wez
Differential Revision: D9344414
fbshipit-source-id: 8b30b57d979dc45af06676979e75e00b02d2b89b
Summary:
This diff is first in the series to make Eden work on Windows. It includes:
1. HG backing store and Object store, which provides the capability to talk to mercurial and fetch the file and folder contents on Windows.
2. Subprocess and Pipe definition for Windows.
3. The Visual studio solution and projects files to compile Eden and scm datapack.
Few Important points:
1. Most of the changes to existing code is done under a macro EDEN_WIN so that it doesn't impact on other platform.
2. Sqlite is used for caching the fetched contents. We are not using Rocksdb on Windows.
3. The main function only calls some test code and exit after printing the output.
4. The initializeMononoke code is disabled for Windows because it needs Proxygen to talk HTTP. Will enable this once I get Proxygen and other dependencies working.
5. HgImporter pass Windows handles to hg_import_helper as command line args. The code to convert these handles into fds is in a separate diff.
Reviewed By: wez
Differential Revision: D8653992
fbshipit-source-id: 52a3c3750425fb92c2a7158c2c214a9372661e13
Summary:
As a prerequisite to running inode unloading code in a background
queue, use a deterministic executor in TestMount to make sequencing in
unit tests reliable.
Reviewed By: simpkins
Differential Revision: D9323878
fbshipit-source-id: 0b85632c1637a8cf83d6f238675e5b6bbb6923c7
Summary:
Update the hg_import_helper.py code to catch exceptions from `repo.close()`
when we are re-opening the repository after an error. Also set `self.repo` to
`None` before we start, so that it will be left as None if anything goes wrong
either closing or re-opening the repository.
I ran into a situation where `repo.close()` threw an error, and previously
this would leave `hg_import_helper.py` stuck in a bad state since it would
still be pointing to the old repo object. The next time it received a command
it would fail and try to call `self.repo.close()` again, which would still
fail since the repository was already in a halfway closed state.
Now the code will always forget about the old repository object and create a
new object.
Reviewed By: wez
Differential Revision: D9419109
fbshipit-source-id: 15bb296ba19d9d3d2a2b90169bf25b0e8e197c1f
Summary:
Fix the output of `eden debug journal` after changing the Thrift API
to pass paths in bytes instead of strings.
Reviewed By: simpkins
Differential Revision: D9403416
fbshipit-source-id: acd9d0f8b241d11295a48fa11f9cc59bd86381f2
Summary: This commit let `HgImporter` pick up configuration values from `EdenConfig`.
Reviewed By: chadaustin
Differential Revision: D9346293
fbshipit-source-id: cb63f7d13a86058e9bf076eddb52212560a64cb1
Summary:
While destroying JournalDelta entries, unzip the chain as we go and
destroy each entry one by one. This prevents the stack from
overflowing on long JournalDelta chains and should fix some flaky
tests.
Reviewed By: wez
Differential Revision: D9355365
fbshipit-source-id: 31af124d318ca5d7a84314b707e1b3c71b2ccaa9
Summary:
For O(1) stack space JournalDelta destruction, we need to check
whether a reference-counted pointer has a unique refcount. It looks at
first glance like shared_ptr<T>::unique() gives us that, but it uses a
relaxed load and is now deprecated.
Sadly, the easiest thing is to make a new smart pointer class. (It
happens to be slightly more efficient, requiring one fewer heap
allocation per entry in the chain.)
Reviewed By: simpkins
Differential Revision: D9355314
fbshipit-source-id: 8c782ba9e0ec27fae90325079c199e1b82df88fa
Summary:
This is part of "the great r-valuification of folly::Future":
* This is something we should do for safety in general.
* Several of folly::Future's methods are lvalue-qualified even though they act as though they are rvalue-qualified, that is, they provide a postcondition that says, in effect, callers should act as though the method invalidated its `this` object (regardless of whether that invalidation was actual or logical).
* This violates the C++ principle to "Express ideas directly in code" (see Core Guidelines), and generally makes it more confusing for callers as well as hiding the actual semantics from tools (linters, compilers, etc.).
* This dichotomy and confusion has manifested itself by some failures around D7840699 since lvalue-qualification hides that operation's move-out semantics - leads to some use of future operations that are really not correct, but are not obviously incorrect.
* The goal of rvalueification is to make sure methods that are logically rvalue-qualified are actually rvalue-qualified, which forces callsites to acknowledge that rvalueification, e.g., `std::move(f).ensure(...)` instead of `f.ensure(...)`. This syntactic change in the callsites forces callers to acknowledge the method's rvalue semantics.
This diff started as a Codemod, then required manual fixes. Here were the codemod steps:
* expr.ensure(...) ==> std::move(expr).ensure(...) // if expr is not already an xvalue
* expr->ensure(...) ==> std::move(*expr).ensure(...)
Note: operator precedence of that last step is safe - no need to parenthesize `expr`. Reason: `->` binds more tightly than unary `*`.
Reviewed By: yfeldblum
Differential Revision: D9332070
fbshipit-source-id: 882121fe82c05fdb196ce676db686b6bc254974b
Summary:
Now that timestamps are read from the inode metadata table, and users
aren't likely to run a pre-metadata-table version, the timestamp data
in the overlay header's no longer needs to be written. So remove that
code which has the bonus of making unloading faster.
Reviewed By: wez
Differential Revision: D9318044
fbshipit-source-id: 27a9a9ee954003940209819466932237a81f8929
Summary:
The Eden CLI allowed setting the `EDEN_CONFIG_DIR` environment variable to
control where to find the `.eden` state directory, as an alternative to the
`--config-dir` command line argument. However, nothing currently appears to
use this variable. Therefore remove this functionality for now to help
simplify the code.
Reviewed By: wez
Differential Revision: D9355514
fbshipit-source-id: c64afb54f599924945573b07bc6d91b346978ea8
Summary: Fix a few minor issues that `mypy --strict` complains about.
Reviewed By: wez
Differential Revision: D9355653
fbshipit-source-id: af63825721fc964b7713df68e8618b595c91561d
Summary:
In the CLI, rename the "Config" class to "EdenInstance". This class
represents all state about a particular edenfs instance. It provides APIs for
making thrift calls to edenfs, for creating and destroying checkouts, and
generally does much more than just managing configuration.
Renaming it to "EdenInstance" also helps clarify that it is distinct from the
configuration tracked in the user's ~/.edenrc and the /etc/eden directory.
This change purely renames internal class and variable names and should not
affect any user-visible functionality.
Reviewed By: wez
Differential Revision: D9355515
fbshipit-source-id: ba5d4c3b753c6eb12a3783306dcd29e85fea3f52
Summary:
This is part of "the great r-valuification of folly::Future":
* This is something we should do for safety in general.
* Several of folly::Future's methods are lvalue-qualified even though they act as though they are rvalue-qualified, that is, they provide a postcondition that says, in effect, callers should act as though the method invalidated its `this` object (regardless of whether that invalidation was actual or logical).
* This violates the C++ principle to "Express ideas directly in code" (see Core Guidelines), and generally makes it more confusing for callers as well as hiding the actual semantics from tools (linters, compilers, etc.).
* This dichotomy and confusion has manifested itself by some failures around D7840699 since lvalue-qualification hides that operation's move-out semantics - leads to some use of future operations that are really not correct, but are not obviously incorrect.
* The goal of rvalueification is to make sure methods that are logically rvalue-qualified are actually rvalue-qualified, which forces callsites to acknowledge that rvalueification, e.g., `std::move(f).unit(...)` instead of `f.unit(...)`. This syntactic change in the callsites forces callers to acknowledge the method's rvalue semantics.
Codemod changes:
* expr.unit(...) ==> std::move(expr).unit(...) // if expr is not already an xvalue
* expr->unit(...) ==> std::move(*expr).unit(...)
Note: operator precedence of that last step is safe - no need to parenthesize `expr`. Reason: `->` binds more tightly than unary `*`.
Reviewed By: LeeHowes
Differential Revision: D9347419
fbshipit-source-id: 2773365f3793d977f2bad1c0a85ef394633e7d2c
Summary:
Watchman's Eden integration has a bug where the combination of
Watchman querying Eden for overlapping delta ranges ("give me changes
between X and Y, now changes between X+1 and Y+1") and Eden eliding
redundant change events ("add-modify-remove" -> []) results in
Watchman sometimes reporting that a file exists in its final
subscription update when it no longer does.
The fix is to never elide events, even for files that were added and
removed in the same sequence. To continue to support Watchman's `new`
flag, track whether a file existed at the beginning and end of a
journal delta.
Reviewed By: wez
Differential Revision: D9304964
fbshipit-source-id: f34c12b25f2b24e3a0d46fc94aa428528f4c5098
Summary:
The setcon() failure is not actionable or interesting, so don't log it
to stdout at Eden startup.
Reviewed By: wez
Differential Revision: D9344467
fbshipit-source-id: 68435c8f22c228f2fbb86f37c2b1874723934169
Summary:
Most of the time, the age cutoff for unloads is extraneous, so default
it to 0 in order to unload everything.
Reviewed By: wez
Differential Revision: D9329161
fbshipit-source-id: a241630ea5c069b6f9dafc9f021e39365b96b3bf
Summary: This commit adds two configurable value for setting path to client certificate and a flag to control Mononoke integration.
Reviewed By: chadaustin
Differential Revision: D9303157
fbshipit-source-id: 2f44d55d17b567655157a5f4b6f52e9468dda234
