WofWca authored 1 month ago and Cecylia Bocovich committed 3 weeks ago

This migrates from using `atomic.LoadInt64` on `int64`
to making the `clients` field itself `atomic.Int64`.
Also `count` now takes `*tokens_t` by reference,
which fixes a linter warning.

It's not clear to me why it warned about this,
but I simplified it anyway.

And `failedConnectionCount`.
Convert the `int` / `uint` to `atomic.Int32` / `atomic.Uint32`.
The race was discovered by running a proxy with the `-race` flag.

Although it does not look like that there are situations
where it is critical to use a mutex, because it's only used
when performing rendezvous with a proxy, which doesn't happen
too frequently,
let's still do it just to be sure.

Some of the changes do not appear to have a potential race condition,
so there it is purely a refactor,
while in others (e.g. in broker.go and in proxy/lib/snowflake.go)
we do use the same variable from multiple threads / functions.

WofWca authored 1 month ago and Cecylia Bocovich committed 3 weeks ago

It appears that there is no need for the `isClosing` var at all:
we can just `close(c.sendMoreCh)` to ensure that it doesn't block
any more `Write()`s.

This is a follow-up to
!144.
Related:
!524.

Our metrics were undercounting client polls by missing the case where
clients are matched with a snowflake but receive a timeout before the
snowflake responds with its answer. This change adds a new metric,
called client-snowflake-timeout-count, to the 24 hour broker stats and a
new "timeout" status label for prometheus metrics.

WofWca authored 1 month ago and shelikhoo committed 3 weeks ago

For the summary log and for Prometheus metrics.

Log output example:

> In the last 1h0m0s, there were 7 completed successful connections. 2 connections failed. Traffic Relayed ↓ 321 KB (0.10 KB/s), ↑ 123 KB (0.05 KB/s).

Closes #40377

To test that the broker responds with a proxy answer if available, have
only one valid client offer to ensure metrics will always be in the
first multiple of 8.

This fixes a race condition in tests where sometimes snowflake matching
happens before enough snowflakes get added to the heap.

WofWca authored 1 month ago and shelikhoo committed 1 month ago

Related: #40378.

Fixes a bug where socksAcceptLoop was reusing the same client config
when processing arguments from multiple SOCKS connections, causing
different bridge lines to clobber each other.

Our SQS tests were not concurrency safe and we hadn't noticed until now
because we were processing incoming SQS queue messages sequentially
rather than in parallel.

This fix removes the log output checks, which were prone to error
anyway, and relies instead on gomock's expected function calls and
strategic use of the context cancel function for each test.

We're losing a lot of messages from the broker SQS queue because they
are exceeding their maximum lifetime before being read and processed by
the broker. This change speeds up that process by increasing the size of
messagesChn and processing the messages within a go routine.

WofWca authored 3 months ago and shelikhoo committed 1 month ago

This is a hack, and I'm not entirely sure how it works,
but it appears to work, at least somewhat.
See #40419 (comment 3141855).

We only use the IP part of the address.

WofWca authored 1 month ago and Cecylia Bocovich committed 1 month ago

As per tpo/anti-censorship/pluggable-transports/snowflake!502 (comment 3159902)

.

The comment was added in c28c8ca4,
and got outdated apparently after
83c01565.

Signed-off-by: Cecylia Bocovich <cohosh@torproject.org>

WofWca authored 1 month ago and Cecylia Bocovich committed 1 month ago

This is related to
tpo/anti-censorship/pluggable-transports/snowflake#40230.

The initial MR that closed that issue,
tpo/anti-censorship/pluggable-transports/snowflake!391,
was not semantically correct, because `DataChannelTimeout`
starts after the client has already received the answer.

After
tpo/anti-censorship/pluggable-transports/snowflake!498 (comment 3156256)
the code became not only semantically incorrect,
but also functionally incorrect because now if this timeout is hit
by the proxy, the client is guaranteed to be gone already.
This commit fixes it, by lowering the timeout.

This addresses a suggestion in
tpo/anti-censorship/pluggable-transports/snowflake#40447.

This also closes
tpo/anti-censorship/pluggable-transports/snowflake#40381
and supersedes
tpo/anti-censorship/pluggable-transports/snowflake!415.

Renovate Bot authored 1 month ago and Cecylia Bocovich committed 1 month ago

chore(deps): update gitlab.torproject.org/tpo/anti-censorship/pluggable-transports/ptutil digest to efaf4e0

Signed-off-by: Cecylia Bocovich <cohosh@torproject.org>

Renovate Bot authored 1 month ago and Cecylia Bocovich committed 1 month ago

Signed-off-by: Cecylia Bocovich <cohosh@torproject.org>

Renovate Bot authored 1 month ago and Cecylia Bocovich committed 1 month ago

Signed-off-by: Cecylia Bocovich <cohosh@torproject.org>

Matches the observed timeout for CDN77, based on user reports.
tpo/anti-censorship/pluggable-transports/snowflake#40446