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 for connection cleanup #6364

Merged
merged 2 commits into from
Jul 16, 2024
Merged

Conversation

eddyashton
Copy link
Member

Combination of recent connection cleanup changes were dangerous, as noticed by the ASAN build. We were modifying a collection while iterating over it:

for (auto& [id, idle_time]: idle_times)
{
  ...
  stop(id);
  ...
}

bool stop(ConnectionID id)
{
  ...
  idle_times.erase(id);
  ...
}

We could potentially drop this erasure entirely, I think it's a safe but unnecessary step. But to be a little safer, less likely to spam logs in an edge case, I've retained it - callers of stop(id) now call idle_times.erase(id) themselves.

@eddyashton eddyashton requested a review from a team as a code owner July 15, 2024 20:32
@maxtropets
Copy link
Contributor

We were modifying a collection while iterating over it:

Is it possible we still modify it simultaneously from both on_timer() and message dispatch?

@achamayou achamayou added this pull request to the merge queue Jul 16, 2024
Merged via the queue into microsoft:main with commit 6d739fb Jul 16, 2024
17 checks passed
@achamayou achamayou deleted the daily_san_connections branch July 16, 2024 07:17
@eddyashton
Copy link
Member Author

We were modifying a collection while iterating over it:

Is it possible we still modify it simultaneously from both on_timer() and message dispatch?

No - these both happen on the same thread, as part of libuv's dispatch loop.

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.

3 participants