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

Add (flexible) hook for initial connection opening event #1508

Closed
jnoordsij opened this issue Aug 26, 2024 · 6 comments · Fixed by #1515
Closed

Add (flexible) hook for initial connection opening event #1508

jnoordsij opened this issue Aug 26, 2024 · 6 comments · Fixed by #1515

Comments

@jnoordsij
Copy link

jnoordsij commented Aug 26, 2024

Is your feature request related to a problem? Please describe.
MySQL allows for configuration of various behavior-changing settings on a per-connection/session basis, which is done through queries. This includes for example setting collation (SET NAMES 'utf8mb4';), setting timezone (SET time_zone='Europe/Amsterdam';) or locale support (SET lc_time_names = 'nl_NL';). Configuring these preferably should be done once at connection startup to ensure they are used throughout the entire lifetime of the connection and cause as little overhead as possible.

In my usecase I would like to set the (connection) timezone, however I think the problem can be generalized to any of such settings.

Describe the solution you'd like
Have an event hook that allows for safely and easily executing option-setting queries once after the first opening of a connection. This could be a generic event that allows arbitrary code execution, or could be some more subtle mechanism that only allows passing e.g. a list of statements to be executed once.

Describe alternatives you've considered

  • In ConnectionString way to specify timezone? #487 there was some discussion about possibly adding this to ConnectionString; given the wide variety of possibly interesting options, I agree that adding this here is probably too much overhead; however it still can be considered.
  • I've tried to implement this using the StateChange event, however I run into troubles with some race conditions where the next query already seems to be trying to execute when the timezone setting query is still active, causing exceptions. Implementation roughly looks like this:
    As a workaround, something similar to this can currently be used
connection.StateChange += (_, args) =>
    {
        // Only consider transition from Closed/Connecting to Open to denote the 'start' of a new connection
        if (args.CurrentState == ConnectionState.Open && (args.OriginalState is ConnectionState.Closed or ConnectionState.Connecting))
            {
                conn.Execute("SET time_zone = 'UTC'");
            }
    };
  • Manually opening the connections at some point in the application logic, followed by setting the aforementioned settings with queries. I can't however manage to let this play nicely with passing the connection to Dapper.

Additional context

Final note
If there are any suggestions/alternative approaches on how to subtly set time_zone or other settings with existing code, please let me know!

@bgrainger
Copy link
Member

connection.StateChange += async (_, args) =>

This adds an async void handler, which should be avoided.

Use a synchronous event handler and your race condition should be eliminated: the event handler will run before the connection is returned to the caller.

As per #519 (comment) I still recommend MySqlConnection.StateChange as the way to solve this in user code (without needing to add a new but equivalent hook to MySqlConnector).

@bgrainger
Copy link
Member

With the move towards MySqlDataSource (for DI) and MySqlDataSourceBuilder (to configure it), it could make sense to provide a new method on the builder that registers an "at startup" hook, relieving the user from having to add a .StateChanged event handler everywhere a new MySqlConnection is instantiated.

This could support both sync and async overloads, helping avoid the problem in the OP.

Reopening this issue as a placeholder for considering this new API.

@bgrainger bgrainger reopened this Aug 26, 2024
@jnoordsij
Copy link
Author

This adds an async void handler, which should be avoided.

Ah yeah, that makes a lot of sense, thanks for pointing that out!

Reopening this issue as a placeholder for considering this new API.

👍

@scastria
Copy link

scastria commented Aug 27, 2024

I definitely agree that this is needed. We connect to RDS MySQL using IAM DB Auth tokens. Therefore, the ProvidePasswordCallback is very important. However, when we connect, we are set with a default reader role. For write connections, it would be nice to specify what role the connection should switch to upon making a connection. Rather than force the developer to execute a set role command, it would be nice if a startup command can be specified as described above. In Python MySQL connector, this is a property called init_command.

I will try to use the StateChange event for now.

@bgrainger
Copy link
Member

Npsql has a callback for when a physical connection is established: npgsql/npgsql#3032. (Couldn't tell if it integrates that with NpgsqlDataSource or not; it may predate it.)

bgrainger added a commit that referenced this issue Oct 6, 2024
This differs from the existing StateChanged event in that:

- it supports an async callback
- it's only invoked when a connection is opened
- it provides information about new vs existing and whether the connection was reset

Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
bgrainger added a commit that referenced this issue Oct 13, 2024
This differs from the existing StateChanged event in that:

- it supports an async callback
- it's only invoked when a connection is opened
- it provides information about new vs existing and whether the connection was reset

Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
@bgrainger
Copy link
Member

Added in 2.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants