search

trinodb / trino

Official repository of Trino, the distributed SQL query engine for big data, formerly known as PrestoSQL (https://trino.io)
https://trino.io
Apache License 2.0
10.51k stars 3.03k forks source link

WKBReader is not thread-safe. Remove static field #24045

Closed umartin closed 2 weeks ago

umartin commented 2 weeks ago

Description

This is a follow up to PR #23894 As discussed in the original PR the WKBReader is not thread-safe. This PR removes the static instance.

Additional context and related issues

PR #23894

Release notes

(x) This is not user-visible or is docs only, and no release notes are required. ( ) Release notes are required. Please propose a release note for me. ( ) Release notes are required, with the following suggested text:

wendigo commented 2 weeks ago

You could use ThreadLocals for that to benefit from cached instance but being thread safe

martint commented 2 weeks ago

Avoid thread locals if possible. They are an anti-pattern for that purpose, and don’t play well with virtual threads.

wendigo commented 2 weeks ago

@martint I'm not sure whether this is still true for VT once the object monitor stuff gets merged but I agree that it should be avoided.

ebyhr commented 2 weeks ago

@umartin Please follow the commit message guideline from next time: https://trino.io/development/process#pull-request-and-commit-guidelines-