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

fix connection timeouts in pool #27

Merged
merged 5 commits into from
Sep 6, 2015
Merged

Conversation

jettify
Copy link
Member

@jettify jettify commented Sep 1, 2015

Should fix #25 and #16

@asvetlov please review.

@@ -125,6 +125,10 @@ def acquire(self):
conn = self._free.popleft()
assert not conn.closed, conn
assert conn not in self._used, (conn, self._used)
# drop if connection timedout
Copy link
Member

Choose a reason for hiding this comment

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

Would you move the patch into _fill_free_pool() method?
It makes code more clear from my perspective.
Just iterate over _free list and drop all timed-out connections.

@jettify
Copy link
Member Author

jettify commented Sep 5, 2015

Done.

# iterate over free connections and remove timeouted ones
free_size = len(self._free)
n = 0
while n < free_size:
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Still not perfect: you are removing connection from deque and pushing it back if it is not closed (which is most likely happens).

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think about instead pop/appendleft use rotate? This method does not perform unnecessary removals and just relink underlying linked list:

while n < free_size:
    conn = self._free[-1]
    if conn._reader.at_eof():
        self._free.pop()
        conn.close()
    else:
        self._free.rotate()
    n += 1

Copy link
Member

Choose a reason for hiding this comment

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

Looks good!

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.

ERROR:aiohttp.web:Error handling request
3 participants