ClickHouse / ClickHouse/clickhouse-java

DataTypeConverter usage has potential thread-safety issues

Abierto
#2,728 1 comentario 0 reacciones 0 asignados Ver en GitHub
bug client-api-v2
Lenguaje dominante
Java
Estrellas
1.6k
Forks
636
Merge medio
2 d 23 h
PR fusionados (30 d)
29

Descripción

## 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..

Guía de contribución

Abrir la guía de contribución

Línea de trabajo

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.

Escrito por el modelo de indexación a partir del texto del issue.

Evaluación

Stack tecnológico
java
Área
api, backend
Tipo de issue
Error
Dificultad
4/5
Tiempo estimado
3-5 días
Estado de actividad
Tranquilo
Claridad
Necesita aclaración
Aptitud para principiantes
38/100

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.