Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect connection metrics tracking on instances pending shutdown #214

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jan 31, 2024

While a spectator server instance was pending shutdown, it could start reporting endlessly rising user counts until its actual shutdown. The reason for this was questionable logic in ConcurrentConnectionLimiter.

When I wrote that thing I used try...finally in OnConnectedAsync() and OnDisconnectedAsync() because I wanted to have multiple returns in the body of the method and sitll call await next() at the end. However I appear to have also forgotten that it means that await next() would be called even if the preceding code threw an exception, and then the exception would be thrown anyway.

This in the case of StatefulUserHubs led to the following sequence of events:

  1. ConcurrentConnectionLimiter.OnConnectedAsync() fires
  2. It dies on its innards, but because of the finally, calls await next() anyway
  3. Inside await next() is the hub's connection method, in particular LoggingHub.OnConnectedAsync(), which increments the datadog connected user gauge
  4. Then after the finally, because any exception that was thrown was not caught, it will get rethrown, interrupting the connection flow
  5. After this, OnDisconnectedAsync() will not get called for this connection, leading the counter to not decrement back down from the increment in point (3)

leading to the runaway increase in reported user counts.

To fix, stop using try...finally and split the blocks of code that want early-returns to separate methods instead.

While a spectator server instance was pending shutdown, it could start
reporting endlessly rising user counts until its actual shutdown. The
reason for this was questionable logic in `ConcurrentConnectionLimiter`.

When I wrote that thing I used `try...finally` in `OnConnectedAsync()`
and `OnDisconnectedAsync()` because I wanted to have multiple returns in
the body of the method and sitll call `await next()` at the end. However
I appear to have also forgotten that it means that `await next()` would
be called _even if the preceding code threw an exception_, and then _the
exception would be thrown anyway_.

This in the case of `StatefulUserHub`s led to the following sequence of
events:

1. `ConcurrentConnectionLimiter.OnConnectedAsync()` fires
2. It dies on its innards, but because of the `finally`, calls `await
   next()` anyway
3. Inside `await next()` is the hub's connection method, in particular
   `LoggingHub.OnConnectedAsync()`, which increments the datadog
   connected user gauge
4. Then after the finally, because any exception that was thrown was
   not caught, it will get _rethrown_, interrupting the connection flow
5. After this, `OnDisconnectedAsync()` will not get called for this
   connection, leading the counter to not decrement back down from the
   increment in point (3)

leading to the runaway increase in reported user counts.

To fix, stop using `try...finally` and split the blocks of code that
want early-returns to separate methods instead.
@peppy peppy merged commit 1e6947b into ppy:master Feb 1, 2024
2 checks passed
@peppy peppy changed the title Fix incorect connection metrics tracking on instances pending shutdown Fix incorrect connection metrics tracking on instances pending shutdown Feb 1, 2024
@peppy peppy self-requested a review February 1, 2024 09:07
@bdach bdach deleted the fix-broken-tracking branch February 1, 2024 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants