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

sendable + general cleanup (linting) #10

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AntVil
Copy link

@AntVil AntVil commented Dec 5, 2024

basically only Sendable + minor cleanup

i wanted to ask what best to do in Sources/ClickHouseNIO/Connection.swift:74:

/// Set to true if `close()` was called
private var _isClosed = false

it's currently not Sendable because of this:

warning: stored property '_isClosed' of 'Sendable'-conforming class 'ClickHouseConnection' is mutable; this is an error in the Swift 6 language mode

@patrick-zippenfenig
Copy link
Owner

I'm not sure whether the ClickHouse connection should be marked as Sendable. The primary concern is the lack of guarantees that it functions correctly across multiple threads. While I haven’t explicitly tested this, the ClickHouseVapor adapter uses an address pool to preemptively avoid such issues.

Making it conform to Sendable requires significant care. At present, there are no unit tests to validate potential concurrency problems. Simply following the compiler’s guidance and marking classes as Sendable won’t address the underlying issues. For example, wrapping _isClosed = false with a NIOLock might suppress compiler warnings, but it doesn’t fundamentally resolve the problem.

If you intend to proceed with making the adapter Sendable, you’ll need to implement unit tests to verify its correctness under concurrency conditions.

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