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.
Ian Jackson
Nick Mathewson
eta
trinity-1686a
Ian Jackson
solanav
Trinity Pointard
Christian Grigis
Dimitris Apostolou