I have Plans for this macro. In particular:
* I have a wip branch which tests that the Builder can be
deserialised from an empty config (ie, that config reading
of a config with a blank section for this item works).
* I think we should autogenerate $Config::builder(),
and promote that, rather than $ConfigBuilder::default().
This macro could do that.
This macro is kind of derive-y. Also it has a test in it, and failing
to call it could allow bugs to exist, as well as missing bits of API.
Putting it next to the structs makes it easy to see that it's actually
been called.
Discovered by a test case in my local tree. The test case was
macro-generated by an extension of impl_standard_builder (which
macro istself currently awaiting review, arti!499)
Have also sent an MR to update the upstream docs
https://github.com/jean-airoldie/humantime-serde/pull/8
This new section describes how much variance we accept when it comes
to expired and not-yet-valid directory documents. (Currently, the
only ones where this matters for are consensus documents and
authority certificates.) A document that is invalid by no more than
these tolerances is not _live_, but it can still be used.
These tolerances serve two purposes:
* First, they allow clients to run with a little more clock skew
than they would tolerate otherwise.
* Second, they allow clients to survive the situation where the
authorities are unable to reach a consensus for a day or two.
Compare with Tor's REASONABLY_LIVE_TIME and NETWORKSTATUS_ALLOW_SKEW
constants; also compare with proposal 212.
Closes#412.
I'm doing this per discussion, so that we can have it be part of the
TorConfig later on, and not break stuff as we change the Mistrust
API to have a builder.
This change, unfortunately, results in a little more internal
complexity and duplicated code in arti and arti-client. I've marked
those points with TODOs.
This is derived from the environment, not the configuration file: We
might not want to trust the configuration file until we've decided
whether we like its permissions.
For reference, the git source for this crate (and the others in its
workspace) currently lives in my personal github account (ijackson).
If this fork turns out to be long-lived and gains features and/or
users, it would be good to move it to a gitlab somewhere.
I have granted Nick crate ownership on the crates.io system.
* Builders additionally derive: Debug, Serialize, Deserialize.
* Validated structs no longer derive: Serialize, Deserialize
and all related attributes deleted.
* As a consequence, all the `#[serde(deny_unknown_fields)]`
are gone. That means that right now unknown fields are totally
ignored. This is good for compatibility but poor for useability.
Doing something better here is arti#417, in progress.
* As a consequence, delete tor_dirmgr::retry::default_parallelism.
(The default value was already duplicated into a builder attr.)
And drop the ad-hoc orport() method. This brings FallbackDir's
orports field in line with our list builder API.
The general semver note in "configuation" seems to cover most of this.
The new API is (roughly) as discussed in
https://gitlab.torproject.org/tpo/core/arti/-/issues/451
This is quite a large commit and it is not convenient to split it up.
It contains the following changes:
* Redo the list builder and accessor macros implemnetation,
including docs and tests.
* Change uses of define_list_config_builder. In each case:
- Move the docs about the default value to the containing field.
- Remove the other docs (which were just recapitulations, and
are now not needed since the ListBuilder is no longer public).
- Rewmove or replace `pub` in the define_list_builder_helper call,
so that the builder is no longer public.
- Change the main macro call site to use define_list_builder_helper.
- Add a call to define_list_builder_accessors.
* Make the module `list_builder` pub so that we have somewhere to
put the overview documentation.
* Consequential changes:
- Change `outer.inner().replace(X)` to `outer.set_inner(X)`
- Consequential changes to imports (`use` statements).
Document that this can contain either a string for expansion, or a
literal PathBuf not for expansion.
Rename the `from_path` method to `new_literal`: a very important
difference is whether it gets expanded - less important than the Rust
type. Also, now it takes `Into<PathBuf>`, which avoids a needless
clone.
(We don't change the API in `arti-client` because
`&tempfile::Tempdir()` doesn't implement `Into<PathBuf>`, so
`arti-client` has to have some new `as_ref` calls.)
Provide accessors `as_unexpanded_str` and `as_literal_path`. The
deserialisation already makes this part of the stable API,l so not
pvoding accessors seems just obstructive. They are useful for tests,
too.
Add tests for the new entrypoints, and for deserialisation of both
variants from TOML (via config, or directly) and JSON.
The `[patch]` approach causes the tree not to build when used as a
dependency, unless the `[patch]` is replicated into the depending
project.
Instead, replace our `derive_builer =` dependencies with a reference
to a specific git commit:
perl -i~ -pe 'next unless m/^derive_builder/; s#"(0\.11\.2)"#{ version = "$1", git = "https://github.com/ijackson/rust-derive-builder", rev = "ba0c1a5311bd9f93ddf5f5b8ec2a5f6f03b22fbe" }#' crates/*/Cargo.toml
Note that the commitid has changed. This is because derive_builder is
in fact a workspace of 4 crates. 3 of them are of interest to arti
itself (the 4th exists only for testing). So the same "add git
revision" treatment had to be done to the `derive_builder` and
`derive_builder_macro` crates. Each dependency edge involves a new
commit in the derive_builder workspace, since we can't create a git
commit containing its own commitid. (We want to use commits, rather
than a branch, so that what we are depending on is actually properly
defined, and not subject to the whims of my personal github
namespace.)
There are no actual code changes in derive_builder.
Use sub_builder. We must do something special for defaults.
This involves moving the actual default values for retry_bootstrap and
retry_microdescs into config.rs, since they need to access the fields
of the un-built version of the structure. (An alternative would be to
generate "weak setters" which do not override previous settings, but
derive_builder does not offer to generate them and that seems
overkill.)
Instead, everyone should use DownloadScheduleBuilder.
The new() method would in any case be useless in a moment, since we're
going to embed DownloadScheduleBuilder in the NetworkConfig, not
DownloadSchedule.
The call sites in the tests are all about to change again.
This is an automated change made with a perl one-liner and verified
with grep -L and grep -l.
Some warnings are introduced with this change; they will be removed
in subsequent commits.
See arti#208 for older discussion on this issue.
NetworkConfigBuilder needs to not contain any validated structs, so
that its serde does not expose the validated details.
AuthorityListBuilder is what ought to go here - and it contains
Vec<AuthorityBuilder>, not Vec<Authority>. As a consequence, many
places now deal with AuthorityBuilder, rather than Authority.
Now the network fallbacks configuration wants to Deserialize
a Vec<FallbackDirBuilder>, rather than validated Vec<FallbackDir>.
Methods on FallbackListBuilder are as per
https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/462#note_2797697
mutatis mutandi for the fact that this struct has only fallbacks in it.
This commitid is the current head of my MR branch
https://github.com/colin-kiegel/rust-derive-builder/pull/253https://github.com/ijackson/rust-derive-builder/tree/field-builder
Using the commitid prevents surprises if that branch is updated.
We will require this newer version of derive_builder. The version
will need to be bumped again later, assuming the upstream MR is merged
and upstream do a release containing the needed changes.
We will need the new version of not only `derive_builder_core` (the
main macro implementation) but also`derive_builder` for a new error
type.
Not all of these strictly need to be bumped to 0.2.0; many could go
to 0.1.1 instead. But since everything at the tor-rtcompat and
higher layers has had breaking API changes, it seems not so useful
to distinguish. (It seems unlikely that anybody at this stage is
depending on e.g. tor-protover but not arti-client.)
The guard manager is responsible for handing out the first hops of
tor circuits, keeping track of their successes and failures, and
remembering their states. Given that, it makes sense to store this
information here. It is not yet used; I'll be fixing that in
upcoming commits.
Arguably, this information no longer belongs in the directory
manager: I've added a todo about moving it.
This commit will break compilation on its own in a couple of places;
subsequent commits will fix it up.
The various background daemon tasks that `arti-client` used to spawn are
now handled inside their respective crates instead, with functions
provided to spawn them that return `TaskHandle`s.
This required introducing a new trait, `NetDirProvider`, which steals
some functionality from the `DirProvider` trait to enable `tor-circmgr`
to depend on it (`tor-circmgr` is a dependency of `tor-dirmgr`, so it
can't depend on `DirProvider` directly).
While we're at it, we also make some of the tasks wait for events from
the `NetDirProvider` instead of sleeping, slightly increasing
efficiency.
There are two reasons why the DynFilter newtype might be needed:
1. To impl Default. But we don't need it to impl Default since we can
have an accessor which does the defaulting.
2. To hide the API. But this is usrely an unstable API.
Just writing Arc<dyn> gets rid of a lot of unnecessary boilerplate and
conversion code.
This is a revised version of !397; it implements a scheduling system for
periodic tasks that can be externally controlled, and then uses the
external control aspect to implement a basic dormant mode (#90).
More technically, the scheduling system consists of a `Stream` that
periodic tasks are expected to embed in a `while` loop or similar, a
way for tasks themselves to choose how long to wait until the stream
next yields a result, and a handle to control this outside of the task.
Instead of requiring a `Box<dyn Isolation>`, it now takes either a
`Box<dyn Isolation>`, or an arbitrary `T` that implements
`Isolation`.
This API still allows the user to pass in a `Box<dyn Isolation>` if
that's what they have, but it doesn't require them to Box the
isolation on their own.
Part of #414.
Now we use NetParams. That implies making its constructor public,
which I think it fine.
This is related to #413 but is far from completing that ticket.
This handwritten conversion function omitted a field. There was
nothing to spot this mistake.
IMO this shows why these particular types ought not to use builders,
but instead, should cause API breaks when things change.
Adding this line here to explicitly fix the bug, although we are about
to abolish this function completely almost right away.
This has the different syntax for builder field attributes than what I
originally proposed in my MR, and which therefore is in the pinned
branch.
My upstream MR for the field attributes feature was morged:
https://github.com/colin-kiegel/rust-derive-builder/issues/239
This has the humantime_serde::option module, which we have upstreamed
and are about to switch to.
The remaining dependency with version = "1" is going to be removed
in a moment.
Replace the recapitulation of TorClientConfig fields in ArtiConfig and
instead just have it contain one. This is part of #374.
The conversions from ArtiConfig back to ArtiConfigBuilder and
TorClientConfigBuilder would need to change, but, since we don't want
them anyway,
No longer impl Deserialize for ArtiConfig. (As per #371 this will
want to become a private type.)
No longer impl From<ArtiConfig> for ArtiConfigBuilder and
TorClientConfigBuilder. And abolish tests of that code.
(This all has to be in one commit, because previously
ArtiConfig::tor_client_config used the validated-to-builder config
retcon.)
I used
git-grep -P '\#\[serde\((?!default|deny_unknown)'
to find places where I needed to add additional attributes on the
builder method fields.
This is currently a bit duplicative, but when #371 is completely done,
the validated (non-builder) configs won't need to be Deserialize any
more.
This is part of #371 and #372.
We are going to want to specify custom attributes on fields of the
builder struct. This feature was missing from derive_builder.
This commitid is the current head of my MR branch
https://github.com/colin-kiegel/rust-derive-builder/pull/237https://github.com/ijackson/rust-derive-builder/tree/builder-field-attrs
Using the commitid prevents surprises if that branch is updated.
We will require this newer version of derive_builder. The version
will need to be bumped again later, assuming the upstream MR is merged
and upstream do a release containing the needed changes.
Previously they returned an Arc, which wasn't necessary unless the
client actually _wanted_ a new Arc.
This would be an API break, except that these functions are marked
'experimental-api', so semver does not apply; nonetheless I've noted
the break in semver_status.md, just in case we care.
Closes#369
This makes using the `PreferredRuntime` the first-class option inside
`arti-client`, freeing users who don't want to think about runtimes from
having to do so.
`TorClient::create_unbootstrapped` and `builder` now automatically
use this runtime, leaving only `builder_custom` for users who wish to
manually specify a runtime.
This lets us clean up the docs a lot: mentions of using custom runtimes
are now relegated to nearer the end of the crate-level documentation,
and we mostly just link to `tor_rtcompat`'s docs to explain more there.
Instead, we take some more time to explain how you use the builder API
to create clients synchronously.
Other doc cleanups included getting rid of the explanation of `TorAddr`
in the main crate-level doc; this is already well-documented elsewhere,
and is something users should discover organically later.
fixes arti#326
`StateMgr` got a new `unlock()` method that does what it says on the
tin. We now call it from `bootstrap()` using the new
`util::StateMgrUnlockGuard`, which works in a manner similar to the
`BoolResetter` from `tor_dirmgr`.
(A decent small little task in future might be to unify these types in
some sort of general arti utility crate?)
closes arti#335
This lint is IMO inherently ill-conceived.
I have looked for the reasons why this might be thought to be a good
idea and there were basically two (and they are sort of contradictory):
I. "Calling ‘.clone()` on an Rc, Arc, or Weak can obscure the fact
that only the pointer is being cloned, not the underlying data."
This is the wording from
https://rust-lang.github.io/rust-clippy/v0.0.212/#clone_on_ref_ptr
It is a bit terse; we are left to infer why it is a bad idea to
obscure this fact. It seems to me that if it is bad to obscure some
fact, that must be because the fact is a hazard. But why would it be
a hazard to not copy the underlying data ?
In other languages, faliing to copy the underlying data is a serious
correctness hazard. There is a whose class of bugs where things were
not copied, and then mutated and/or reused in multiple places in ways
that were not what the programmer intended. In my experience, this is
a very common bug when writing Python and Javascript. I'm told it's
common in golang too.
But in Rust this bug is much much harder to write. The data inside an
Arc is immutable. To have this bug you'd have use interior mutability
- ie mess around with Mutex or RefCell. That provides a good barrier
to these kind of accidents.
II. "The reason for writing Rc::clone and Arc::clone [is] to make it
clear that only the pointer is being cloned, as opposed to the
underlying data. The former is always fast, while the latter can
be very expensive depending on what is being cloned."
This is the reasoning found here
https://github.com/rust-lang/rust-clippy/issues/2048
This is saying that *not* using Arc::clone is hazardous.
Specifically, that a deep clone is a performance hazard.
But for this argument, the lint is precisely backwards. It's linting
the "good" case and asking for it to be written in a more explicit
way; while the supposedly bad case can be written conveniently.
Also, many objects (in our codebase, and in all the libraries we use)
that are Clone are in fact simply handles. They contain Arc(s) (or
similar) and are cheap to clone. Indeed, that is the usual case.
It does not make sense to distinguish in the syntax we use to clone
such a handle, whether the handle is a transparent Arc, or an opaque
struct containing one or more other handles.
Forcing Arc::clone to be written as such makes for code churn when a
type is changed from Arc<Something> to Something: Clone, or vice
versa.
This patch removes the EventStream associated type and the Runtime
parameter. The Runtime parameter wasn't actually used for anything,
and the EventStream was easy enough to replace with a BoxStream in
this case.
Also replaced DirBootstrapEvents with a BoxStream to avoid tying
anything to our backend.
Try to make the `hook-tcp` example a bit easier to read by
adding/changing comments, and renaming the lifetimes for
`async_trait`-generated trait methods.
This is the first one where anyhow::Error impl AsRef<dyn StdError>
We want this because we want to add error reporting functionality
which works with all kinds of errors, which means we need an
anyhow::Error which can be vieweed as a StdError.
(The alternative would be to deref at the call sites of
report_and_exit, making it less ergonomic.)
anyhow 1.0.23 is from November 2019.
The new `BootstrapBehavior` enum controls whether an unbootstrapped
`TorClient` will bootstrap itself automatically (`Ondemand`) when an
attempt is made to use it, or whether the user must perform
bootstrapping themselves (`Manual`).
The `lazy-init` example shows how you could write a simple
`get_tor_client()` function that used a global `OnceCell` to share
a Tor client across an entire application with this API.
closes arti#278
Ian Jackson
Nick Mathewson
eta
Samanta Navarro
trinity-1686a
Ian Jackson
solanav
Trinity Pointard
Christian Grigis
Dimitris Apostolou
Yuan Lyu