ClickHouse / ClickHouse/clickhouse-java

DataTypeConverter usage has potential thread-safety issues

Open
#2,728 1 comment 0 reactions 0 assignees View on GitHub
bug client-api-v2
Dominant language
Java
Stars
1.6k
Forks
636
Avg merge
2d 23h
Merged PRs (30d)
29

Description

## Description

_This is not an explicit bug report in the sense that I haven't experienced a problem (yet?), it is just something I bumped into while browsing the code_.

`DataTypeConverter` has a [javadoc comment stating that it is not thread safe](https://github.com/ClickHouse/clickhouse-java/blob/main/client-v2/src/main/java/com/clickhouse/client/api/internal/DataTypeConverter.java#L25). For what it's worth, I have also confirmed thread safety problems because I ended up using the `DataTypeConverter.INSTANCE` in production (before I realised that it was labelled as non thread safe) which caused some bugs.

After fixing the bugs in my project, I was curious about the fact that a static singleton `DataTypeConverter.INSTANCE` was even declared in the `clickhouse-java` repo (why have a singleton instance if it is not thread safe, in an otherwise thread safe library?). Of course this class is in a package named `internal`, so it's not really my business... :) Still, I did notice that `DataTypeConverter.INSTANCE` is used in [two](https://github.com/ClickHouse/clickhouse-java/blob/8da0b0c9aa7995b9fa839b56861a28047f71076a/client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/MapBackedRecord.java#L48) [places](https://github.com/ClickHouse/clickhouse-java/blob/8da0b0c9aa7995b9fa839b56861a28047f71076a/client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/AbstractBinaryFormatReader.java#L90) which makes me wonder if it could be the source of (other / future / not-yet-encountered) problems..

Contributor guide

Open the contributing guide

Research direction

Start with the javadoc and implementation of client-v2/src/main/java/com/clickhouse/client/api/internal/DataTypeConverter.java, then inspect its uses in MapBackedRecord.java and AbstractBinaryFormatReader.java. Trace the mutable state and existing tests to establish the intended thread-safety contract; done should include coverage that exercises the shared INSTANCE safely or documents why it cannot be shared.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
api, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.