Several fixes for the connection_pool subpackage #208

oleg-jukovec · 2022-08-18T11:13:58Z

The patchset contains a number of different fixes for the connection_pool package. I tried to make the changes in small commits so that it would be easier to review them. I recommend to review the changes one by one commit.

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

Tests (see documentation for a testing package)
Changelog (see documentation for changelog format)
Documentation (see documentation for documentation style guide)

The patch fixes PREFER_RW/PREFER_RO usage. It also make comments in Go-style. See `Const` section in `Go Doc Comments` guide [1]. 1. https://go.dev/doc/comment Part of #208

Part of #208

It's enough to get `box.info` table once in ConnectionPool.getConnectionRole(). Part of #208

After the patch all errors in the connection_pool subpackage start with a lowercase letter [1]. 1. https://github.com/golang/go/wiki/CodeReviewComments#error-strings Part of #208

We return a role of an instance from `GetPoolInfo()` call. It is unexpected that Role constants are not a part of the public API. Part of #208

We add all connections into ConnectionPool.anyPool, but `MasterRole` connection only in ConnectionPool.rwPool and `ReplicaRole` connections only in ConnectionPool.roPool. As a result `UnknownRole` connections appears only in the `anyPool`. See `setConnectionToPool` implementation. So we need to close connections from the `anyPool` instead of `roPool` + `rwPool`. Part of #208

* Duplicates could be added to a list of connections, which then cannot be deleted. * The delete function uses an address from a connection instead of argument value. It may lead to unexpected errors. Part of #208

After the patch ConnectionPool.getNextConnection() does not return (nil, nil). It always return a connection or an error. The check Connection.ConnectedNow() does not have sence because the connection may be closed right after the call. The code just complicates the logic and does not protect against anything. A chain of two atomic operations IsEmpty() + GetNextConnection() wrong because it leads too a race condition. Part of #208

We need to use copies of slices, not just pointers to them. It helps to avoid unexpected changes. Part of #208

If we create a connection immediately after closing previous, then it can to lead to too frequent connection creation under some configurations [1] and high CPU load. It will be expected to recreate connection with OptsPool.CheckTimeout frequency. 1. #136 Part of #208

The ConnectionPool.checker() goroutine may still work some time after ConnectionPool.Close() call. It may lead to re-open connection in a concurrent closing pool. The connection still opened after the pool is closed. The patch adds RWLock to protect blocks which work with anyPool, roPool and rwPool. We don't need to protect regular requests because in the worst case, we will send a request into a closed connection. It can happen for other reasons and it seems like we can't avoid it. So it seems loke an expected behavior. Closes #208

The ConnectionPool.checker() goroutine may still work some time after ConnectionPool.Close() call. It may lead to re-open connection in a concurrent closing pool. The connection still opened after the pool is closed. The patch adds RWLock to protect blocks which work with anyPool, roPool and rwPool. We don't need to protect regular requests because in the worst case, we will send a request into a closed connection. It can happen for other reasons and it seems like we can't avoid it. So it seems like an expected behavior. Closes #208

The ConnectionPool.checker() goroutine may still work some time after ConnectionPool.Close() call. It may lead to re-open connection in a concurrent closing pool. The connection still opened after the pool is closed. The patch adds RWLock to protect blocks which work with anyPool, roPool and rwPool. We don't need to protect regular requests because in the worst case, we will send a request into a closed connection. It can happen for other reasons and it seems like we can't avoid it. So it is an expected behavior. Closes #208

After the patch ConnectionPool.getNextConnection() does not return (nil, nil). It always return a connection or an error. The check Connection.ConnectedNow() does not have sence because the connection may be closed right after the call. The code just complicates the logic and does not protect against anything. A chain of two atomic operations IsEmpty() + GetNextConnection() wrong because it leads too a race condition. Part of #208

We need to use copies of slices, not just pointers to them. It helps to avoid unexpected changes. Part of #208

If we create a connection immediately after closing previous, then it can to lead to too frequent connection creation under some configurations [1] and high CPU load. It will be expected to recreate connection with OptsPool.CheckTimeout frequency. 1. #136 Part of #208

The ConnectionPool.checker() goroutine may still work some time after ConnectionPool.Close() call. It may lead to re-open connection in a concurrent closing pool. The connection still opened after the pool is closed. The patch adds RWLock to protect blocks which work with anyPool, roPool and rwPool. We don't need to protect regular requests because in the worst case, we will send a request into a closed connection. It can happen for other reasons and it seems like we can't avoid it. So it is an expected behavior. Closes #208

AnaNek

Thank you for fixing connection_pool! Some of my comments are questions for understanding my mistakes.

connection_pool/round_robin.go

connection_pool/round_robin_test.go

connection_pool/connection_pool.go

* Duplicates could be added to a list of connections, which then cannot be deleted. * The delete function uses an address from a connection instead of argument value. It may lead to unexpected errors. Part of #208

After the patch ConnectionPool.getNextConnection() does not return (nil, nil). It always return a connection or an error. The check Connection.ConnectedNow() does not have sence because the connection may be closed right after the call. The code just complicates the logic and does not protect against anything. A chain of two atomic operations IsEmpty() + GetNextConnection() wrong because it leads too a race condition. Part of #208

We need to use copies of slices, not just pointers to them. It helps to avoid unexpected changes. Part of #208

The ConnectionPool.checker() goroutine may still work some time after ConnectionPool.Close() call. It may lead to re-open connection in a concurrent closing pool. The connection still opened after the pool is closed. The patch adds RWLock to protect blocks which work with anyPool, roPool and rwPool. We don't need to protect regular requests because in the worst case, we will send a request into a closed connection. It can happen for other reasons and it seems like we can't avoid it. So it is an expected behavior. Part of #208

An user can specify duplicate addresses in ConnectionPool. We cannot support multiple connections to the same address due to ConnectionPool.GetPoolInfo() implementation without breaking backward compatibility. So we need to skip duplicates. Closes #208

An user can specify duplicate addresses for a ConnectionPool. We cannot support multiple connections to the same address due to the ConnectionPool.GetPoolInfo() implementation without breaking backward compatibility. So we need to skip duplicates. Closes #208

AnaNek

LGTM.

DifferentialOrange

Should be ok

The patch fixes PREFER_RW/PREFER_RO usage. It also make comments in Go-style. See `Const` section in `Go Doc Comments` guide [1]. 1. https://go.dev/doc/comment Part of #208

Part of #208

It's enough to get `box.info` table once in ConnectionPool.getConnectionRole(). Part of #208

After the patch all errors in the connection_pool subpackage start with a lowercase letter [1]. 1. https://github.com/golang/go/wiki/CodeReviewComments#error-strings Part of #208

We return a role of an instance from `GetPoolInfo()` call. It is unexpected that Role constants are not a part of the public API. Part of #208

We add all connections into ConnectionPool.anyPool, but `MasterRole` connection only in ConnectionPool.rwPool and `ReplicaRole` connections only in ConnectionPool.roPool. As a result `UnknownRole` connections appears only in the `anyPool`. See `setConnectionToPool` implementation. So we need to close connections from the `anyPool` instead of `roPool` + `rwPool`. Part of #208

* Duplicates could be added to a list of connections, which then cannot be deleted. * The delete function uses an address from a connection instead of argument value. It may lead to unexpected errors. Part of #208

After the patch ConnectionPool.getNextConnection() does not return (nil, nil). It always return a connection or an error. The check Connection.ConnectedNow() does not have sence because the connection may be closed right after the call. The code just complicates the logic and does not protect against anything. A chain of two atomic operations IsEmpty() + GetNextConnection() wrong because it leads too a race condition. Part of #208

We need to use copies of slices, not just pointers to them. It helps to avoid unexpected changes. Part of #208

If we create a connection immediately after closing previous, then it can to lead to too frequent connection creation under some configurations [1] and high CPU load. It will be expected to recreate connection with OptsPool.CheckTimeout frequency. 1. #136 Part of #208

The ConnectionPool.checker() goroutine may still work some time after ConnectionPool.Close() call. It may lead to re-open connection in a concurrent closing pool. The connection still opened after the pool is closed. The patch adds RWLock to protect blocks which work with anyPool, roPool and rwPool. We don't need to protect regular requests because in the worst case, we will send a request into a closed connection. It can happen for other reasons and it seems like we can't avoid it. So it is an expected behavior. Part of #208

Overview The release adds support for the latest version of the queue package with master-replica switching. Breaking changes There are no breaking changes in the release. New features Support the queue 1.2.1 (#177). ConnectionHandler interface for handling changes of connections in ConnectionPool (#178) Execute, ExecuteTyped and ExecuteAsync methods to ConnectionPool (#176) ConnectorAdapter type to use ConnectionPool as Connector interface (#176) An example how to use queue and connection_pool subpackages together (#176) Bugfixes Mode type description in the connection_pool subpackage (#208) Missed Role type constants in the connection_pool subpackage (#208) ConnectionPool does not close UnknownRole connections (#208) Segmentation faults in ConnectionPool requests after disconnect (#208) Addresses in ConnectionPool may be changed from an external code (#208) ConnectionPool recreates connections too often (#208) A connection is still opened after ConnectionPool.Close() (#208) Future.GetTyped() after Future.Get() does not decode response correctly (#213) Decimal package use a test function GetNumberLength instead of a package-level function getNumberLength (#219) Datetime location after encode + decode is unequal (#217)

Overview The release adds support for the latest version of the queue package with master-replica switching. Breaking changes There are no breaking changes in the release. New features Support the queue 1.2.1 (#177). ConnectionHandler interface for handling changes of connections in ConnectionPool (#178). Execute, ExecuteTyped and ExecuteAsync methods to ConnectionPool (#176). ConnectorAdapter type to use ConnectionPool as Connector interface (#176). An example how to use queue and connection_pool subpackages together (#176). Bugfixes Mode type description in the connection_pool subpackage (#208). Missed Role type constants in the connection_pool subpackage (#208). ConnectionPool does not close UnknownRole connections (#208). Segmentation faults in ConnectionPool requests after disconnect (#208). Addresses in ConnectionPool may be changed from an external code (#208). ConnectionPool recreates connections too often (#208). A connection is still opened after ConnectionPool.Close() (#208). Future.GetTyped() after Future.Get() does not decode response correctly (#213). Decimal package use a test function GetNumberLength instead of a package-level function getNumberLength (#219). Datetime location after encode + decode is unequal (#217).

Overview The release adds support for the latest version of the queue package with master-replica switching. Breaking changes There are no breaking changes in the release. New features Support the queue 1.2.1 (#177). ConnectionHandler interface for handling changes of connections in ConnectionPool (#178). Execute, ExecuteTyped and ExecuteAsync methods to ConnectionPool (#176). ConnectorAdapter type to use ConnectionPool as Connector interface (#176). An example how to use queue and connection_pool subpackages together (#176). Bugfixes Mode type description in the connection_pool subpackage (#208). Missed Role type constants in the connection_pool subpackage (#208). ConnectionPool does not close UnknownRole connections (#208). Segmentation faults in ConnectionPool requests after disconnect (#208). Addresses in ConnectionPool may be changed from an external code (#208). ConnectionPool recreates connections too often (#208). A connection is still opened after ConnectionPool.Close() (#208). Future.GetTyped() after Future.Get() does not decode response correctly (#213). Decimal package use a test function GetNumberLength instead of a package-level function getNumberLength (#219). Datetime location after encode + decode is unequal (#217). Wrong interval arithmetic with timezones (#221). Invalid MsgPack if STREAM_ID > 127 (#224). queue.Take() returns an invalid task (#222).

oleg-jukovec added a commit that referenced this pull request Aug 18, 2022

code health: move NewPrepared to public code block

e47a27a

Part of #208

oleg-jukovec added a commit that referenced this pull request Aug 18, 2022

code health: remove duplicate box.info check

d66e959

It's enough to get `box.info` table once in ConnectionPool.getConnectionRole(). Part of #208

oleg-jukovec added a commit that referenced this pull request Aug 18, 2022

bugfix: move Role constants into the public API

f3d72c7

We return a role of an instance from `GetPoolInfo()` call. It is unexpected that Role constants are not a part of the public API. Part of #208

oleg-jukovec force-pushed the oleg-jukovec/connection_pool_fixes branch from 384023f to 2f3421a Compare August 18, 2022 11:51

oleg-jukovec added a commit that referenced this pull request Aug 18, 2022

bugfix: protect addresses from external changes

a05a595

We need to use copies of slices, not just pointers to them. It helps to avoid unexpected changes. Part of #208

oleg-jukovec force-pushed the oleg-jukovec/connection_pool_fixes branch from 2f3421a to 93315de Compare August 18, 2022 11:58

oleg-jukovec force-pushed the oleg-jukovec/connection_pool_fixes branch from 93315de to d22723a Compare August 18, 2022 12:05

oleg-jukovec marked this pull request as ready for review August 18, 2022 13:43

oleg-jukovec requested review from AnaNek and DifferentialOrange August 18, 2022 13:43

oleg-jukovec added a commit that referenced this pull request Aug 18, 2022

bugfix: protect addresses from external changes

4709c8c

We need to use copies of slices, not just pointers to them. It helps to avoid unexpected changes. Part of #208

oleg-jukovec force-pushed the oleg-jukovec/connection_pool_fixes branch from d22723a to e5cd08b Compare August 18, 2022 18:52

oleg-jukovec mentioned this pull request Aug 22, 2022

api: add ConnectionHandler to the connection_pool #210

Merged

3 tasks

AnaNek reviewed Aug 23, 2022

View reviewed changes

connection_pool/connection_pool.go Outdated Show resolved Hide resolved

oleg-jukovec added a commit that referenced this pull request Aug 23, 2022

bugfix: protect addresses from external changes

8ed7a1c

We need to use copies of slices, not just pointers to them. It helps to avoid unexpected changes. Part of #208

oleg-jukovec force-pushed the oleg-jukovec/connection_pool_fixes branch from 4e7b1c3 to 43cac08 Compare August 25, 2022 08:57

oleg-jukovec force-pushed the oleg-jukovec/connection_pool_fixes branch from 43cac08 to 5e94fd7 Compare August 25, 2022 09:00

oleg-jukovec force-pushed the oleg-jukovec/connection_pool_fixes branch from 5e94fd7 to 6dee596 Compare August 25, 2022 09:02

AnaNek approved these changes Aug 25, 2022

View reviewed changes

DifferentialOrange approved these changes Aug 26, 2022

View reviewed changes

oleg-jukovec merged commit 5801dc6 into master Aug 29, 2022

oleg-jukovec deleted the oleg-jukovec/connection_pool_fixes branch August 29, 2022 08:59

oleg-jukovec added a commit that referenced this pull request Aug 29, 2022

code health: move NewPrepared to public code block

6835690

Part of #208

oleg-jukovec added a commit that referenced this pull request Aug 29, 2022

code health: remove duplicate box.info check

22dfe17

It's enough to get `box.info` table once in ConnectionPool.getConnectionRole(). Part of #208

oleg-jukovec added a commit that referenced this pull request Aug 29, 2022

bugfix: move Role constants into the public API

844f523

We return a role of an instance from `GetPoolInfo()` call. It is unexpected that Role constants are not a part of the public API. Part of #208

oleg-jukovec added a commit that referenced this pull request Aug 29, 2022

bugfix: protect addresses from external changes

5c0de20

We need to use copies of slices, not just pointers to them. It helps to avoid unexpected changes. Part of #208

oleg-jukovec mentioned this pull request Oct 4, 2022

Release 1.9.0 #220

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several fixes for the connection_pool subpackage #208

Several fixes for the connection_pool subpackage #208

oleg-jukovec commented Aug 18, 2022 •

edited

Loading

AnaNek left a comment

AnaNek left a comment

DifferentialOrange left a comment

Several fixes for the connection_pool subpackage #208

Several fixes for the connection_pool subpackage #208

Conversation

oleg-jukovec commented Aug 18, 2022 • edited Loading

AnaNek left a comment

Choose a reason for hiding this comment

AnaNek left a comment

Choose a reason for hiding this comment

DifferentialOrange left a comment

Choose a reason for hiding this comment

oleg-jukovec commented Aug 18, 2022 •

edited

Loading