Mysql\Pool::setCharset() runs forever

[douggr]

// 001_connect.php
Amp\run(function() {
    $db = new \Mysql\Pool(DSN);

    // getConfig() added to my source: return Pool::$config;
    echo $db->getConfig()->charset , "\n";

    $db->setCharset("latin1_general_ci");

    echo $db->getConfig()->charset , "\n";

    $db->close();
});

// php 001_connect.php

awkwardly it runs forever.

If I yield $db->setCharset("latin1_general_ci"); it's runs, but I still get an exception with message 'Connection not ready, cannot send any packets' in Connection.php:1647'.

In other way if I yield $db->query("SET NAMES 'latin1' COLLATE 'latin1_general_ci'"); (replacing setCharset()) it runs flawlessly.

Looks like Pool::$connections is never filled upon __construct, only if you use $db = (yield new Mysql\Pool(DSN)); then Pool::$connectios has 1 item (as expected?), but still can't use $db->setCharset directly, I needed to yield $db->setCharset("latin1_general_ci");.

// finally...
<?php
require './example_bootstrap.php';

Amp\run(function() {
    $db = (yield new Mysql\Pool(DSN));

    echo $db->getConfig()->charset , "\n"; // utf8mb4

    yield $db->setCharset("latin1_general_ci");

    echo $db->getConfig()->charset , "\n"; // latin1
    $db->close();
});

Thoughts?

$ php -v
PHP 5.6.5-1 (cli) (built: Jan 27 2015 15:59:26) 

[douggr]

P.S: The unmodified version of 001_connect runs pretty nice in PHP7.

If it's expected to run in 7, just close this one, I'm fine :) Great job btw.

$ php -v
PHP 7.0.0-dev (cli) (built: Feb 15 2015 23:06:04) 

[rdlowrey]

@douggr I believe the reason you're getting this result is ...

When you create a new Pool instance it schedules the connection to be setup in the next tick of the event loop. When you use yield on the new pool instance the generator passed to Amp\run() won't resume at that location until the next tick so it "just works." Without that artificial pause the connection isn't allowed to initialize internally before the subsequent offending call.

@bwoebi you generally want to return a promise from $pool->setCharset() for this reason. Anything someone can possibly wait on with yield should return a Promise.

In the case of the Mysql\Pool where you have multiple connections this is probably as simple as:

$promises = [];
foreach ($this->connections as $conn) {
    if ($conn->alive()) {
        $promises[] = $conn->setCharset($charset, $collate);
    }
}

return all($promises);

[bwoebi]

@rdlowrey I don't think you should wait on setCharset(). It basically is just setting a connection property. It has no return value and it always should be transparently resolved...

@douggr thanks for the report, I'll look at it this evening.

[rdlowrey]

I don't think you should wait on setCharset()

That's fine if you want to do it transparently and not return a promise, but if so you just need to track the internal connection state yourself so the operation doesn't error out if it's invoked before the connection is established.

@douggr Just as an FYI ... in the future yielding something like the mysql pool instance will result in an error as it's not actually a "yieldable" thing (Promise, Generator, null). So it's really only a temporary fix. Note that if you want a future-proof way to work around this problem you can always use the "pause" yield command key inside any generator function to yield control for the specified number of milliseconds like so:

run(function() {
    echo "tick\n";
    yield "pause" => 1000;
    echo "tick\n";
    yield "pause" => 1000;
    echo "tick\n";
});

[bwoebi]

@rdlowrey Yes, I'll look into it now...

But a setCharset() is just a configuration thing and shouldn't throw any exceptions nor return any success value. So promise is not needed.

[douggr]

Thanks for the feedback guys.

@rdlowrey Yes, I understand the amp workflow, have been using it a while ago. I'm about to open-source a project using amphp goodies :) But's offtopic for now.

@bwoebi Although I still have no idea where's the problem is, I realized that it's not related setCharset() but to the Pool and/or Connection.

You can't just connect and then close the connection if no one query is executed because the event loop was not started. Thats said, so in a case an app connect to the database and executes without querying the db it'll run into this issue.

A workaround I'm using for now is query('select 1') just to get it up and running. It's ugly :)

[bwoebi]

I found the issue, which is basically a missing check. Will fix shortly, but I've found another issue while working on this :-) (indefinite loop)

[bwoebi]

Fixed via efddc156b2e5f0a8adf98bfa6d249b4132f793e8 // That other issue in reality was an uv-related issue which was already fixed in amphp/amp but not yet tagged (sigh).