#186 Support for MaxClientsError for connection and pool #262
Conversation
…d#186 When a new connection tries to get connected to the redis server and it has been configured with a maximum number of connections, the client will get a `MaxClientsError` exception if this limit is reached out. Also if a pool initialization with a `min_size` that can meet the requirements - beucase of that limit u others issues - the pool creation will return also a `MaxClientsError`.
pfreixes
changed the title
| @@ -394,6 +394,9 @@ def _fill_free(self, *, override_min): | |||
| self._acquiring += 1 | |||
| try: | |||
| conn = yield from self._create_new_connection(address) | |||
| # check the healthy of that connection, if | |||
| # something went wrong just trigger the Exception | |||
| yield from conn.execute('ping') | |||
There was a problem hiding this comment.
No really happy with that change - to much implicit -, but it is needed a deterministic way to check that the pool can be created with that min_size requirements. Should we keep this way to check that a connection is not really opened or try something more sophisticated (hard problem)?
Also have in mind that the code that is above when the kw override_min is True, should be following the same pattern than here?
Codecov Report
@@ Coverage Diff @@
## master #262 +/- ##
==========================================
+ Coverage 96.88% 96.95% +0.06%
==========================================
Files 57 58 +1
Lines 7450 7486 +36
Branches 587 591 +4
==========================================
+ Hits 7218 7258 +40
+ Misses 172 168 -4
Partials 60 60
Continue to review full report at Codecov.
|
|
Looks like the test suite executed by appveryor has raised an interesting buggy behavior with asyncio. If there are no waiters waiting in a stream and the stream gets first some data and later a close connection from the other peer, with the current implementation the client wont be able to grab the buffer data that was sent first. Having always an exception notifying that the connection was closed. IMHO a bug in asyncio. Articulate a fix in BTW I will raise this conversation to the Python async community to get feedback, I will work with a fix ASAP . |
|
Oh, that while not self._reader.at_eof():
try:
data = yield from self._reader.read(MAX_CHUNK_SIZE)
except asyncio.CancelledError:
break
except Exception as exc:
# XXX: for QUIT command connection error can be received
# before response
logger.error("Exception on data read %r", exc, exc_info=True)
break
if data == b'' and self._reader.at_eof():
logger.debug("Connection has been closed by server")
breakI think we should do the same here as we do with MaxClientsError — close connection with exception. diff --git a/aioredis/connection.py b/aioredis/connection.py
index b31c886..65f6746 100644
--- a/aioredis/connection.py
+++ b/aioredis/connection.py
@@ -166,9 +166,12 @@ class RedisConnection(AbcConnection):
# XXX: for QUIT command connection error can be received
# before response
logger.error("Exception on data read %r", exc, exc_info=True)
- break
+ self._closing = True
+ self._do_close(exc)
+ return
if data == b'' and self._reader.at_eof():
logger.debug("Connection has been closed by server")
+ # NOTE: probably set ConnectionClosedError here as well.
break
self._parser.feed(data)
while True:Your thoughts? |
|
Correct me if I'm wrong but I would say that this piece of code is something that is really coupled to the transaction command. Trying to make happy all of the paths when one of the commands belonging to a transaction closes the connection. If you change that code the following test will start having issues [1]. My 2cents here. The issue that I've commented before is still possible [2]. And the way to catch them is the piece of code that you commented. But we have two problems here:
[1] https://github.com/aio-libs/aioredis/blob/master/tests/transaction_commands_test.py#L60 |
|
Looks like that the bug [1] is moving ahead in somehow, but obviously it won't get rid of the pain taking into account the multiple python versions that aioredis supports. I'm a bit stuck here, not sure how to tackle this PR having in mind the many factors that are involved:
Any thoughts on how move ahead with all of this? |
|
Here are my quick thoughts:
|
|
Then if you don't mind let's keep this PR frozen till we are able to provide our own So once this is implemented and merged, this PR would get the pipeline in green automatically. Even though my gut feeling says that the transaction command will give us some headaches. I will try to work with the |
Go ahead with this, I don't have much free time for now( |
|
Rebased and merged via #325 |
When a new connection tries to get connected to the redis server and it
has been configured with a maximum number of connections, the client
will get a
MaxClientsErrorexception if this limit is reached out.Also if a pool initialization with a
min_sizethat cant meet therequirements - beucase of that limit u others issues - the pool creation
will return also a
MaxClientsError.