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
Since the only purpose of this function is to make sure that no
bootstrapping task is running, a simple futures:🔒:Mutex
should do the job just fine.
Closes#337.
This commit changes how the `TorClient` type works, enabling it to be
constructed synchronously without initiating the bootstrapping process.
Daemon tasks are still started on construction (although some of them
won't do anything if the client isn't bootstrapped).
The old bootstrap() methods are now reimplemented in terms of the new
create_unbootstrapped() and bootstrap_existing() methods.
This required refactoring how the `DirMgr` works to enable the same sort
of thing there.
closes#293
Instead of declaring a macro that takes vis as an argument, we now
conditionally declare a macro that applies an appropriate visibility.
There's a long comment explaining the rationale here, along with a
couple of other solutions that don't work.
This is closer to what we described in Errors.md.
Also, remove the (sometimes private) Result alias: it was only used in
one or two places, and never exposed in public.
This change lets us make TorError's members unconditionally hidden,
and makes our API a little more consistent (since basically nothing
else is a public field).
At least by default, we should have Error be private, and not expose
it as part of our APIs.
To keep functionality in `arti`, I had to add an `ExitTimeout` error
kind.
For interface consistency, I also re-exported ErrorKind and HasError
from `arti_client`.
The motivation for doing this now is to remove the `#[from]` so we
would spot where operationsl circuit setup failures were handled.
(But it turns out that they are turned into internal errors!)
Perhaps this will want to become a different error type from circmgr
in due course, but for now we simply use a bespoke variant of
TorError.
It will want its own Kind. The TODO in the HasKind impl marks
this (amongst much else here).
This involves making a temporary ErrorKind::TODO. That will continue
to exist until all errors (at least, the ones that make it out to
here) can be properly categorised.
Introducing this will let us work from the top and bottom towards the
middle.
Provide an enum variant to contain the SpawnError and a From impl.
We use `#[from]` here because it doesn't really make sense to attach
any context, as it's not likely to be very relevant.
This commit puts the native-tls crate behind a feature. The feature
is off-by-default in the tor-rtcompat crate, but can be enabled
either from arti or arti-client.
There is an included script that I used to test that tor-rtcompat
could build and run its tests with all subsets of its features.
Closes#300
Having separate types here doesn't justify the (very limited)
benefit of distinguishing between the case where we have created an
executor that we own and the case where we have a handle to an
already-running tokio executor.
Part of #301.
The docs even say this is about stream.
As @nickm writes in
https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/252#note_2771289
we generally call end-to-end connections that are tunneled over Tor
"Streams" to distinguish them from everything else in the Tor
protocols that could possibly be called a "Connection".
That seems to apply here too.
In the usual case, set_isolation_group is awkward.
This is perhaps slightly duplicative with TorClient::isolated_client().
If so then perhaps the *latter* should be abolished.
trinity-1686a
Nick Mathewson
eta
Ian Jackson
Ian Jackson
Trinity Pointard
Christian Grigis
Dimitris Apostolou
Yuan Lyu
Neel Chauhan