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

Connection pool pop() or push() should modify to release idle connection. #99

Closed
xpader opened this issue Oct 2, 2020 · 2 comments
Closed

Comments

@xpader
Copy link

xpader commented Oct 2, 2020

In source code, the pop() method invoke idle->shift() to get top of pool list connection to use, and push return it back to the bottom of list.

In timeoutWatcher first check bottom of the list connection, if not idle will do nothing.

If connection pool has expanded at peak, when the traffic drops back to a low peak, as long as there is one query in each idle time, all connection will never released!!! but only one or two connection is enough now.

So I think idle->push() should change to idle->unshift(), then real idle connection in bottom will be released in time, or timeoutWatcher check top, and pop() change to real idle->pop().

@xpader xpader changed the title Connection pool pop() or push() to change to release idle connection. Connection pool pop() or push() should modify to release idle connection. Oct 2, 2020
@xpader
Copy link
Author

xpader commented Oct 2, 2020

There's another problem,

//In Loop::run() or call()
$result = yield $pool->query("SELECT * FROM some_table LIMIT 1");

//if, not while
if (yield $result->advance()) {
    $row = $result->getCurrent();
    //In this code, if not call advance() or nextResultSet() again, next query will create a new connection.
    //yield $result->nextResultSet();
    print_r($row);
    yield $pool->execute("UPDATE some_table SET counter=counter+1 WHERE id=?", [$row['id']);
}

xpader added a commit to xpader/sql-common that referenced this issue Oct 2, 2020
…ion.

In source code, the pop() method invoke idle->shift() to get top of pool list connection to use, and push return it back to the bottom of list.

In timeoutWatcher first check bottom of the list connection, if not idle will do nothing.

If connection pool has expanded at peak, when the traffic drops back to a low peak, as long as there is one query in each idle time, all connection will never released!!! but only one or two connection is enough now.

So I think idle->push() should change to idle->unshift(), then real idle connection in bottom will be released in time, or timeoutWatcher check top, and pop() change to real idle->pop().

See also amphp/mysql#99
trowski pushed a commit to amphp/sql-common that referenced this issue Nov 9, 2020
…ion. (#3)

In source code, the pop() method invoke idle->shift() to get top of pool list connection to use, and push return it back to the bottom of list.

In timeoutWatcher first check bottom of the list connection, if not idle will do nothing.

If connection pool has expanded at peak, when the traffic drops back to a low peak, as long as there is one query in each idle time, all connection will never released!!! but only one or two connection is enough now.

So I think idle->push() should change to idle->unshift(), then real idle connection in bottom will be released in time, or timeoutWatcher check top, and pop() change to real idle->pop().

See also amphp/mysql#99
@trowski
Copy link
Member

trowski commented Nov 10, 2020

Fixed in amphp/sql-common#3.

@trowski trowski closed this as completed Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants