Skip to content

Exclude server_version from Synchronization mutex#288

Merged
byroot merged 1 commit intotrilogy-libraries:mainfrom
Shopify:nv/exclude-server-version-from-synchronization
Apr 1, 2026
Merged

Exclude server_version from Synchronization mutex#288
byroot merged 1 commit intotrilogy-libraries:mainfrom
Shopify:nv/exclude-server-version-from-synchronization

Conversation

@nvasilevski
Copy link
Copy Markdown
Contributor

Problem

Trilogy#server_version is wrapped by the Synchronization mutex, but it is a pure memory read with no socket I/O. This causes SynchronizationError when instrumentation libraries call server_version from within another synchronized method.

In practice, OpenTelemetry instrumentation (private to our company) wraps Trilogy#query in a span and calls server_version inside the span block to record the server version as a span attribute. Since query is synchronized, calling server_version from within it re-enters the mutex and raises SynchronizationError:

query (synchronized — holds mutex)
  └─ OTel query patch
       └─ client_attributes
            └─ server_version (synchronized — raises SynchronizationError)

This is the same class of problem as #276, which was fixed for closed? in #277.

Why server_version is safe to exclude

server_version returns a fixed char[] copied from the MySQL handshake greeting packet at connect time. It is never modified after initialization and performs no socket I/O.

The C implementation:

// Pure struct read — no socket interaction
static VALUE rb_trilogy_server_version(VALUE self) {
    return rb_str_new_cstr(get_open_ctx(self)->server_version);
}

The field is written once during _connect and never again:

memcpy(ctx->server_version, handshake.server_version, TRILOGY_SERVER_VERSION_SIZE);
ctx->server_version[TRILOGY_SERVER_VERSION_SIZE] = 0;

This makes it identical in safety characteristics to closed?, which is already excluded

server_version is a pure memory read that returns a fixed char[] copied
from the MySQL handshake greeting packet at connect time. It performs no
socket I/O and its value is immutable for the lifetime of the connection.
This makes it identical in safety characteristics to closed?, which is
already excluded from synchronization (see trilogy-libraries#277).

Wrapping server_version in the Synchronization mutex causes
SynchronizationError when it is called re-entrantly from within another
synchronized method. This happens in practice because OpenTelemetry
instrumentation wraps Trilogy#query in a span and calls server_version
inside the span block to record the server version as a span attribute.
Since query is synchronized, calling server_version from within it
re-enters the mutex and raises SynchronizationError.

The C implementation confirms there is no socket interaction:

    static VALUE rb_trilogy_server_version(VALUE self) {
        return rb_str_new_cstr(get_open_ctx(self)->server_version);
    }

The server_version field is written once during _connect:

    memcpy(ctx->server_version, handshake.server_version,
           TRILOGY_SERVER_VERSION_SIZE);
    ctx->server_version[TRILOGY_SERVER_VERSION_SIZE] = 0;

It is never modified again. There is no risk of data races or protocol
corruption from concurrent reads of this field.
@nvasilevski nvasilevski force-pushed the nv/exclude-server-version-from-synchronization branch from 957e71b to a8d4aa1 Compare April 1, 2026 16:20
Copy link
Copy Markdown
Collaborator

@composerinteralia composerinteralia left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Thank you

@byroot byroot merged commit 1583e09 into trilogy-libraries:main Apr 1, 2026
39 checks passed
@nvasilevski nvasilevski deleted the nv/exclude-server-version-from-synchronization branch April 1, 2026 17:59
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