| Commit message (Collapse) | Author | Age | Files | Lines |
|---|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
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.
|
| | |
|
| | |
|
| | |
|
| | |
|
| |
|
|
|
|
|
| |
Getting a non-200 status is no longer a failure condition; it's just
a different kind of answer.
Closes #349.
|
| |\ |
|
| | | |
|
| | |
| |
| |
| | |
So we get a stack trace
|
| | |
| |
| |
| |
| | |
We're about to reuse this and we'll want it to take the higher-level
type. Also it seems more proper like this.
|
| | |
| |
| |
| |
| | |
This isn't complete (see TODO), but it's enough to let us report the
right ErrorKind if something fails to parse.
|
| | |
| |
| |
| |
| |
| |
| |
| | |
It had too many possible Kinds depending on what kind of string had
failed to parse.
I decided to use #[source] here instead of #[from], so that we
would have to explicitly convert these errors where they show up.
|
| | |
| |
| |
| |
| | |
At first I had thought that all sqlite errors would be internal, but that's
not the case.
|
| | | |
|
| |/
|
|
|
|
|
|
|
|
|
|
|
| |
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
|
| |\
| |
| |
| |
| |
| |
| | |
Change deny(clippy::all) to warn(clippy::all).
Closes #338
See merge request tpo/core/arti!306
|
| | |
| |
| |
| | |
Closes #338.
|
| |/
|
|
| |
Found by nightly clippy.
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
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
|
| |
|
|
|
| |
(By our convention, these errors should say what we were trying to
spawn when the error occurred.)
|
| |
|
|
|
| |
This patch makes only minimal changes in lower-level error types:
we have more refactoring to do.
|
| | |
|
| | |
|
| |
|
|
|
|
|
|
|
| |
This failure occurred because our tests use canned data to exercise
the directory state functionality, and the canned consensus has
suddenly become very expired.
There are better fixes possible, but this is a minimal one that
should get CI working on main again.
|
| | |
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
The interface is similar to the one exposed by `arti-client`: it
internally uses postage::watch to give a series of events showing
when a bootstrap status is changing.
Thanks to the existing state/driver separation in the DirMgr design
we don't need much new logic: each download state needs to expose
(internally) how far along it is in its download, which the
bootstrap code passes to the DirMgr if it has changed.
I believe that in the long run, we'll probably want to expose more
(or different) information here, and we'll want to process it
differently. With that in mind, I've made the API for
`DirBootstrapStatus` deliberately narrow, so that we can change its
of its internal later on without breaking code that depends on it.
(The information exposed by this commit is not yet summarized in
`arti-client`.)
Part of #96.
|
| |
|
|
| |
That's what I get for blindly trusting @nickm :p
|
| |\
| |
| |
| |
| |
| |
| | |
Fix ticket 178: Don't use a NetDir until we have microdescriptors for all of our primary guards.
Closes #178
See merge request tpo/core/arti!220
|
| | |
| |
| |
| |
| |
| |
| |
| | |
This prevents a security-failure condition that could happen if our
directory caches don't give us these microdescriptors, but we
nevertheless decide that the directory is usable.
Closes #178
|
| |/
|
|
|
|
|
|
|
|
|
|
|
|
| |
When our current consensus is getting close to being invalid (but
it isn't invalid yet), we try to get a new one. So far, so good.
But we had a bug: when we went to get a new consensus, we'd see that we
had a perfectly fine not-yet-invalid consensus in our cache, reload it,
find that it was ready, and continue!
This patch fixes our behavior: If we have a usable consensus, then
when we reset the bootstrapping process, we ignore any cached consensus.
Fixes bug #274.
|
| | |
|
| |
|
|
| |
Signed-off-by: Tor CI Release <[email protected]>
|
| |
|
|
|
|
|
| |
Previously we didn't retain the value of our cache_usage field when
calling reset() from GetMicrodescsState.
This resolves an XXXX comment.
|
| |\
| |
| |
| |
| | |
Refactor directory events to use a new FlagPublisher mechanism.
See merge request tpo/core/arti!188
|
| | |
| |
| |
| |
| |
| |
| |
| | |
This approach tries to preserve the current interface, but uses a
counter-based event backend to implement a coalescing stream of
events that can be represented as small integers. The advantage
here is that publishing events no longer needs to be a blocking
operation, since there is no queue to fill up.
|
| |/
|
|
|
|
|
|
| |
This warning occurs if we ask for microdescriptors from our local
cache, and our cache gives us something we didn't ask for. It
shouldn't be possible, so let's warn when it occurs.
This patch resolves an XXXX.
|
| |\
| |
| |
| |
| | |
Make most arti-client fields reconfigurable.
See merge request tpo/core/arti!181
|
| | | |
|
| | |
| |
| |
| |
| | |
This is still not as soon as I'd like: a real change here will require
refactoring DirMgr::notify().
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
We can't change the authorities while in-flight: that would be pretty
miserable to implement.
Similarly we can't change the cache while in-flight.
Everything else should be fair game, though there are a couple of tricky
bits. I've tried to document those.
|
| | |
| |
| |
| |
| |
| |
| | |
This patch doesn't actually make anything reconfigurable, but it
does create an API that will tell you "you can't change the value of
that!" If the API looks reasonable, I can start making it possible
to change the values of individual items.
|
| |/ |
|
| |
|
|
|
|
|
|
| |
We want to only use TODO in the codebase for non-blockers, and open
tickets for anything that is a bigger blocker than a TODO. These
XXXXs seem like definite non-blockers to me.
Part of arti#231.
|
| |\
| |
| |
| |
| | |
Don't warn in bootstrap_from_config when error is Error::ManagerDropped
See merge request tpo/core/arti!157
|
| | | |
|
| |/ |
|
| |\ |
|
| | |
| |
| |
| |
| | |
Make sure that we can change elements, and we can reconstruct builders
that give us the same thing.
|
| | | |
|
| |/ |
|
