Skip to content

fix concurrent map write errors with context #1523

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

Merged
merged 3 commits into from
Mar 18, 2025
Merged

fix concurrent map write errors with context #1523

merged 3 commits into from
Mar 18, 2025

Conversation

SpencerTorres
Copy link
Member

Summary

closes #1503

This PR fixes 3 bugs:

  1. A concurrent map write error where clickhouse.WithSettings() maps are re-used between context/calls
  2. Per-query settings will no longer override subsequent queries
  3. An edge case where an empty clickhouse.WithSettings(nil) map could result in a nil write panic

The root cause of #1503 is that maps were being shared between contexts. This is fine when nesting contexts, but there are some cases where the driver will override settings. For example max_execution_time is always overridden when the context has a deadline. Another example is async_insert being changed when using AsyncInsert. When the driver overrides these settings, the settings are persisted for the following queries. This leads to unexpected behavior as well as concurrency issues when overriding settings.

As a fix, the Settings map will now always be copied when read in the driver. This is because the deadline context is almost always modified, and settings are almost always overridden (this will increase garbage but it's negligible).

There were two places in the code where the settings are read but not modified: reading async status and reading user timezone location. I added two functions for these cases so we don't need to make an unnecessary copy.

I added many tests to solidify the behavior of the driver's context wrapper. I also added a test case that forces a concurrent map write issue (which doesn't fail anymore since it's fixed).

Function documentation has been updated too to make it clearer in the future. The code is also a bit more readable and easy to follow when seeing where the Settings map is initialized.

Checklist

  • Unit and integration tests covering the common scenarios were added

Copy link

@abraithwaite abraithwaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is waay cleaner than the slop I wrote! Just suggested a slightly more detailed doc comment but otherwise looks fantastic, thank you!

@SpencerTorres SpencerTorres merged commit 8c6911b into main Mar 18, 2025
13 checks passed
@SpencerTorres SpencerTorres deleted the issue_1503 branch March 18, 2025 17:06
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.

Obscure Concurrent Map Write Panic from Clickhouse-go
2 participants