| 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.
|
| |
|
|
| |
Closes #338.
|
| |
|
|
|
| |
This avoids a future confusion with the new `SpawnBlocking` trait in
async_executors v0.5, and better describes what the trait provides.
|
| |
|
|
|
|
|
|
|
|
|
| |
This took some refactoring, so that I wouldn't need to define 9
different versions of the function. It also required that we change
the behavior of test_with_all_runtimes slightly, so that it asserts
on _any_ failure rather than asserting on most but returning Err()
for others. That in turn required changes to a few of its callers.
There's probably a better way to do all of this macro business, but
this is the best I could find.
|
| | |
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
`tor-rtcompat`'s `TlsConnector` trait previously included a method to
create a TLS-over-TCP connection, which implied creating a TCP stream
inside that method. This commit changes that, and makes the function
wrap a TCP stream, as returned from the runtime's `TcpProvider` trait
implementation, instead.
This means you can actually override `TcpProvider` and have it apply to
*all* connections Arti makes, which is useful for issues like arti#235
and other cases where you want to have a custom TCP stream
implementation.
This required updating the mock TCP/TLS types in `tor-rtmock` slightly;
due to the change in API, we now store whether a `LocalStream` should
actually be a TLS stream inside the stream itself, and check this
property on reads/writes in order to detect misuse. The fake TLS wrapper
checks this property and removes it in order to "wrap" the stream,
making reads and writes work again.
|
| |
|
|
|
|
|
|
| |
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.
|
| | |
|
| | |
|
| |
|
|
| |
These are my fault; I merged the wrong version of !102. :p
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Instead of racily advancing time forward, this commit attempts to rework
how WaitFor works, such that it makes advances when all sleeper futures
that have been created have been polled (by handing the MockSleepRuntime
a Waker with which to wake up the WaitFor).
The above described mechanics work well enough for the double timeout
test, but fail in the presence of code that spawns asynchronous /
background tasks that must make progress before time is advanced for the
test to work properly. In order to deal with these cases, a set of APIs
are introduced in order to block time from being advanced until some
code has run, and a carveout added in order to permit small advances in
time where required.
(In some cases, code needed to be hacked up a bit in order to be made
properly testable using these APIs; the `MockablePlan` trait included in
here is somewhat unfortunate.)
This should fix arti#149.
|
| | |
|
| | |
|
|
|
This will cause some pain for now, but now is really the best time
to do this kind of thing.
|
