Skip to content

Block tarantool.Connect() until a connection is established or the timeout expires if Reconnect option set #436

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

Closed
ImeevMA opened this issue Mar 25, 2025 · 4 comments · Fixed by #437

Comments

@ImeevMA
Copy link

ImeevMA commented Mar 25, 2025

Currently tarantool.Connect() returns a connection if successfully connected to a Tarantool instance, or an error if the connection fails. It also ignores the Reconnect option on the first connection. This differs from the behavior of net.box, which blocks and attempts to reconnect if the reconnect_after option is non-nil. It is suggested that tarantool.Connect() block until the context times out or a connection is established.

ImeevMA added a commit that referenced this issue Mar 26, 2025
This patch makes Connect() retry connection attempts if opts.Reconnect
is greater than 0. The delay between connection attempts is
opts.Reconnect. If opts.MaxReconnects > 0 then the maximum number of
attempts is equal to it, otherwise the maximum number of attempts is
unlimited. Connect() now also blocks until a connection is established,
provided context is cancelled or the number of attempts is exhausted.

Closes #436
ImeevMA added a commit that referenced this issue Mar 26, 2025
This patch makes Connect() retry connection attempts if opts.Reconnect
is greater than 0. The delay between connection attempts is
opts.Reconnect. If opts.MaxReconnects > 0 then the maximum number of
attempts is equal to it, otherwise the maximum number of attempts is
unlimited. Connect() now also blocks until a connection is established,
provided context is cancelled or the number of attempts is exhausted.

Closes #436
ImeevMA added a commit that referenced this issue Mar 26, 2025
This patch makes Connect() retry connection attempts if opts.Reconnect
is greater than 0. The delay between connection attempts is
opts.Reconnect. If opts.MaxReconnects > 0 then the maximum number of
attempts is equal to it, otherwise the maximum number of attempts is
unlimited. Connect() now also blocks until a connection is established,
provided context is cancelled or the number of attempts is exhausted.

Closes #436
@locker
Copy link
Member

locker commented Mar 26, 2025

It'd be really nice if there was an option to make Connect non-blocking. The net.box connector has the wait_connected flag for this. If the user tries to use a connection object (e.g. issue a call) before the connection is established in the background, the function will block.

@oleg-jukovec
Copy link
Collaborator

oleg-jukovec commented Mar 26, 2025

This is non-obvious behavior that is very confusing for users. It can be achieved now in the client code by writing a little code.

We've act that way this before and have reject this behavior.

@locker
Copy link
Member

locker commented Mar 26, 2025

This is non-obvious behavior that is very confusing for users.

We use the Go connector in Aeon (range-based sharding for Tarantool) and for us absence of something like the wait_connected option from net.box is really inconvenient because we don't want to block startup of a router if one of storage replicas is unavailable, for the cluster is still fully functional in this case.

I guess it could be unacceptable to other users of the Go connector so I suggest disabling this feature by default, like it is disabled by default in net.box. OTOH I don't understand how it can be confusing. If you mean #136, when Ping failed after successful Connect, then this problem could be solved by making any connection method (Ping, Call, ...) wait until the connection is established before trying to execute the request. This is what net.box does BTW.

It can be achieved now in the client code by writing a little code.

Solving this problem in the application code doesn't look trivial to me: we'd have to start a background fiber that would call Connect in a loop and provide some kind of synchronization against other fibers trying to use the connection before it's established - all to handle this rare case.

My point is that the situation when a connection hasn't been established yet isn't very different from the situation when it was temporarily broken due to a network error and the connector is trying to reconnect. If the connector supports reestablishing a connection on failure in the background, it should support establishing the initial connection in the background, as well.

@oleg-jukovec
Copy link
Collaborator

We don't want to block startup of a router if one of storage replicas is unavailable, for the cluster is still fully functional in this case.

You could just to start a goroutine or implement the tarantool.Doer interface that creates connection in background if needed. It is not big deal in the Golang.

then this problem could be solved by making any connection method (Ping, Call, ...) wait until the connection is established before trying to execute the request.

And then the logic becomes even more confusing. In general, the request will wait for a connect. In corner cases, this will either dramatically complicate the code of the connector (and it's performance), or the user will still get an error on unlucky disconnect.

My point is that the situation when a connection hasn't been established yet isn't very different from the situation when it was temporarily broken due to a network error and the connector is trying to reconnect.

Yes, it is no different than if the connection is lost immediately after it is established and before a request.

But in practice, it is not intuitive and people cannot use and handle the case properly. In addition, it complicates the connector configuration. Considering that this feature is rarely used, I don't want to make the connector code more complicated and make its behavior more complex and confusing.

ImeevMA added a commit that referenced this issue Mar 27, 2025
This patch makes Connect() retry connection attempts if opts.Reconnect
is greater than 0. The delay between connection attempts is
opts.Reconnect. If opts.MaxReconnects > 0 then the maximum number of
attempts is equal to it, otherwise the maximum number of attempts is
unlimited. Connect() now also blocks until a connection is established,
provided context is cancelled or the number of attempts is exhausted.

Closes #436
ImeevMA added a commit that referenced this issue Mar 27, 2025
This patch makes Connect() retry connection attempts if opts.Reconnect
is greater than 0. The delay between connection attempts is
opts.Reconnect. If opts.MaxReconnects > 0 then the maximum number of
attempts is equal to it, otherwise the maximum number of attempts is
unlimited. Connect() now also blocks until a connection is established,
provided context is cancelled or the number of attempts is exhausted.

Closes #436
ImeevMA added a commit that referenced this issue Mar 27, 2025
This patch makes Connect() retry connection attempts if opts.Reconnect
is greater than 0. The delay between connection attempts is
opts.Reconnect. If opts.MaxReconnects > 0 then the maximum number of
attempts is equal to it, otherwise the maximum number of attempts is
unlimited. Connect() now also blocks until a connection is established,
provided context is cancelled or the number of attempts is exhausted.

Closes #436
ImeevMA added a commit that referenced this issue Mar 27, 2025
This patch makes Connect() retry connection attempts if opts.Reconnect
is greater than 0. The delay between connection attempts is
opts.Reconnect. If opts.MaxReconnects > 0 then the maximum number of
attempts is equal to it, otherwise the maximum number of attempts is
unlimited. Connect() now also blocks until a connection is established,
provided context is cancelled or the number of attempts is exhausted.

Closes #436
ImeevMA added a commit that referenced this issue Mar 28, 2025
This patch makes Connect() retry connection attempts if opts.Reconnect
is greater than 0. The delay between connection attempts is
opts.Reconnect. If opts.MaxReconnects > 0 then the maximum number of
attempts is equal to it, otherwise the maximum number of attempts is
unlimited. Connect() now also blocks until a connection is established,
provided context is cancelled or the number of attempts is exhausted.

Closes #436
ImeevMA added a commit that referenced this issue Mar 28, 2025
This patch makes Connect() retry connection attempts if opts.Reconnect
is greater than 0. The delay between connection attempts is
opts.Reconnect. If opts.MaxReconnects > 0 then the maximum number of
attempts is equal to it, otherwise the maximum number of attempts is
unlimited. Connect() now also blocks until a connection is established,
provided context is cancelled or the number of attempts is exhausted.

Closes #436
ImeevMA added a commit that referenced this issue Mar 28, 2025
This patch makes Connect() retry connection attempts if opts.Reconnect
is greater than 0. The delay between connection attempts is
opts.Reconnect. If opts.MaxReconnects > 0 then the maximum number of
attempts is equal to it, otherwise the maximum number of attempts is
unlimited. Connect() now also blocks until a connection is established,
provided context is cancelled or the number of attempts is exhausted.

Closes #436
oleg-jukovec pushed a commit that referenced this issue Mar 28, 2025
This patch makes Connect() retry connection attempts if opts.Reconnect
is greater than 0. The delay between connection attempts is
opts.Reconnect. If opts.MaxReconnects > 0 then the maximum number of
attempts is equal to it, otherwise the maximum number of attempts is
unlimited. Connect() now also blocks until a connection is established,
provided context is cancelled or the number of attempts is exhausted.

Closes #436
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 a pull request may close this issue.

3 participants