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

api: add context to connection create #333

Merged

Conversation

DerekBum
Copy link

connection.Connect and pool.Connect no longer return non-working connection objects. Those functions now accept context as their first arguments, which user may cancel in process.

connection.Connect will block until either the working connection created (and returned), opts.MaxReconnects creation attempts were made (returns error) or the context is canceled by user (returns error too).

I didn't forget about (remove if it is not applicable):

Closes #136

@DerekBum DerekBum force-pushed the DerekBum-gh-136-add-context-to-connection-create branch 2 times, most recently from bb488f6 to ff7346d Compare September 26, 2023 14:23
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

  1. I've expect that a context could be used with a network connection call. We could use net.DialContext instead of net.Dial.
  2. What do you thing about get rid of the opts.Reconnect logic with timeouts and retries inside the Connect()? We could use just a context object to handle the Connect lifetime and try to make the connection only once.

connection.go Outdated Show resolved Hide resolved
tarantool_test.go Outdated Show resolved Hide resolved
pool/connection_pool.go Outdated Show resolved Hide resolved
pool/connection_pool_test.go Outdated Show resolved Hide resolved
example_test.go Outdated Show resolved Hide resolved
pool/connection_pool.go Outdated Show resolved Hide resolved
@DerekBum
Copy link
Author

We are currently using net.diatTimeout:

go-tarantool/dial.go

Lines 201 to 212 in d8df65d

// dial connects to a Tarantool instance.
func dial(address string, opts DialOpts) (net.Conn, error) {
network, address := parseAddress(address)
switch opts.Transport {
case dialTransportNone:
return net.DialTimeout(network, address, opts.DialTimeout)
case dialTransportSsl:
return sslDialTimeout(network, address, opts.DialTimeout, opts.Ssl)
default:
return nil, fmt.Errorf("unsupported transport type: %s", opts.Transport)
}
}

I don't think we can use context from Connect call in this case (unless we ask the user to use contextWithTimeout to specify DialTimeout field).

@oleg-jukovec
Copy link
Collaborator

oleg-jukovec commented Sep 27, 2023

I don't think we can use context from Connect call in this case (unless we ask the user to use contextWithTimeout to specify DialTimeout field).

We could remove and don't use the opts.DialTimeout since we'll be using a more flexible context object.

@DerekBum DerekBum force-pushed the DerekBum-gh-136-add-context-to-connection-create branch from ff7346d to 83a8e6f Compare October 3, 2023 12:15
@DerekBum
Copy link
Author

DerekBum commented Oct 3, 2023

NB: need to change go.mod and go.sum after the patch from go-openssl.

@DerekBum DerekBum force-pushed the DerekBum-gh-136-add-context-to-connection-create branch 2 times, most recently from 4c4ac1c to e3e28fa Compare October 3, 2023 12:53
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

The code is not the same as in the previous implementation. The previous implementation have a timeout by default. The current code have an infinity timeout.

So we need to use a context default timeout in all tests/examples/inner calls (or pass a context to it).

tarantool.Connect(context.Background(), server, opts)

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
connection.go Outdated Show resolved Hide resolved
connection.go Outdated Show resolved Hide resolved
connection.go Outdated Show resolved Hide resolved
connection.go Outdated Show resolved Hide resolved
ssl_test.go Outdated Show resolved Hide resolved
tarantool_test.go Outdated Show resolved Hide resolved
test_helpers/pool_helper.go Outdated Show resolved Hide resolved
test_helpers/pool_helper.go Outdated Show resolved Hide resolved
dial_test.go Outdated Show resolved Hide resolved
ssl_test.go Outdated Show resolved Hide resolved
@DerekBum DerekBum force-pushed the DerekBum-gh-136-add-context-to-connection-create branch 3 times, most recently from 5a79c7f to 0d93a03 Compare October 6, 2023 13:00
CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
pool/connection_pool.go Outdated Show resolved Hide resolved
pool/connection_pool.go Show resolved Hide resolved
pool/connection_pool.go Outdated Show resolved Hide resolved
ssl.go Outdated Show resolved Hide resolved
tarantool_test.go Outdated Show resolved Hide resolved
@DerekBum DerekBum force-pushed the DerekBum-gh-136-add-context-to-connection-create branch from 0d93a03 to c85bfae Compare October 11, 2023 12:24
CHANGELOG.md Outdated Show resolved Hide resolved
decimal/example_test.go Outdated Show resolved Hide resolved
dial_test.go Outdated Show resolved Hide resolved
dial_test.go Outdated Show resolved Hide resolved
example_custom_unpacking_test.go Outdated Show resolved Hide resolved
pool/connection_pool.go Outdated Show resolved Hide resolved
pool/connection_pool.go Outdated Show resolved Hide resolved
pool/connection_pool.go Outdated Show resolved Hide resolved
shutdown_test.go Outdated Show resolved Hide resolved
tarantool_test.go Outdated Show resolved Hide resolved
@DerekBum DerekBum force-pushed the DerekBum-gh-136-add-context-to-connection-create branch from c85bfae to 7364d75 Compare October 12, 2023 13:36
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

Thank you for the patch!

pool/connection_pool.go Outdated Show resolved Hide resolved
pool/connection_pool.go Outdated Show resolved Hide resolved
@DerekBum DerekBum force-pushed the DerekBum-gh-136-add-context-to-connection-create branch 2 times, most recently from 0c482a9 to 560f58d Compare October 12, 2023 15:48
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
crud/tarantool_test.go Show resolved Hide resolved
tarantool_test.go Outdated Show resolved Hide resolved
@DerekBum DerekBum force-pushed the DerekBum-gh-136-add-context-to-connection-create branch from 560f58d to 387460f Compare October 17, 2023 09:46
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

Please, fix the problem and describe the fix/problem.

--- FAIL: TestAdd_CloseGraceful_concurrent (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x9a8cec]

goroutine 301 [running]:
testing.tRunner.func1(0xc000106400)
	/opt/hostedtoolcache/go/1.13.15/x64/src/testing/testing.go:874 +0x69f
panic(0xbe5d60, 0x111eb00)
	/opt/hostedtoolcache/go/1.13.15/x64/src/runtime/panic.go:679 +0x1b2
github.com/tarantool/go-tarantool/v2/pool.(*ConnectionPool).CloseGraceful(0xc00011e8c0, 0xc95140, 0xc000350520, 0xc00011e8c0)
	/home/runner/work/go-tarantool/go-tarantool/pool/connection_pool.go:335 +0x13c
github.com/tarantool/go-tarantool/v2/pool_test.TestAdd_CloseGraceful_concurrent(0xc000106400)
	/home/runner/work/go-tarantool/go-tarantool/pool/connection_pool_test.go:487 +0x44f
testing.tRunner(0xc000106400, 0xc95148)
	/opt/hostedtoolcache/go/1.13.15/x64/src/testing/testing.go:909 +0x19a
created by testing.(*T).Run
	/opt/hostedtoolcache/go/1.13.15/x64/src/testing/testing.go:960 +0x652
FAIL	github.com/tarantool/go-tarantool/v2/pool	5.065s

@DerekBum
Copy link
Author

DerekBum commented Oct 17, 2023

Yeah, that happened because of this race:

  1. In Add we are adding an endpoint to the map (p.addrs[addr] = e) inside a protected code (with locked p.addrsMutex). But after that we unlock the mutex, call tryConnect and only after that initialize cancel() field for endpoint.
  2. In CloseGraceful we are rlocking the p.addrsMutex and for each saved endpoint calling cancel() (may not be initialized).

So as a fix I did this: inside Add function initialize cancel in the protected part. In case of any error, we just cancel the created context.

Note: there is no problem with ConnectWithOpts, because we can't use the new pool until we return from the function.

`connection.Connect` and `pool.Connect` no longer return non-working
connection objects. Those functions now accept context as their first
arguments, which user may cancel in process.

`connection.Connect` will block until either the working connection
created (and returned), `opts.MaxReconnects` creation attempts
were made (returns error) or the context is canceled by user
(returns error too).

Closes #136
@DerekBum DerekBum force-pushed the DerekBum-gh-136-add-context-to-connection-create branch from 387460f to b402c58 Compare October 17, 2023 14:14
@oleg-jukovec oleg-jukovec merged commit 8075914 into master Oct 18, 2023
22 checks passed
@oleg-jukovec oleg-jukovec deleted the DerekBum-gh-136-add-context-to-connection-create branch October 18, 2023 10:30
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.

v2: restart on connection create
3 participants