fiber: release lock in pool:put(conn)

[romanhabibov]

@ChangeLog

• Fix bug with infinite fiber blocking when using pool:put(conn) and conn:execute() in different fibers (Ensure <connection object>:execute() don't block a fiber infinitely #34).

[LeonidVas]

Hi! Thank you for the patch.
I don't understand, why you choose api as subsystem? You don't change API in the patch.
Please add a detailed description of the situation that could lead to infinite blocking instead of "case in tests".
See some comments below.

[LeonidVas]

Why did you move the second test into a separate commit?

[LeonidVas]

Please, add @ChangeLog for the patchset.

[LeonidVas]

Please give your branches a descriptive name according to SOP(for the future).

[romanhabibov]

Please, add @ChangeLog for the patchset.

Done.

[romanhabibov]

Please give your branches a descriptive name according to SOP(for the future).

Ok.
I thought this naming was in order not to get confused in the branches, if there are a lot of them. And this repository is not very rich in branches.

[romanhabibov]

Why did you move the second test into a separate commit?

Because this test is not about pool connections, but my first patch is about. Also, this test does not fail on master, it was added, because Sanya (@Totktonada) requested it in the issue description.

[LeonidVas]

Hi! This iteration looks better than the previous one.
See some comments below.

[LeonidVas]

Please, add @ChangeLog for the patchset.

Done.

Optional!!!
When the user reads the release notes it will be nice if he understand: how the behavior has been changed. Try to write more detailed description.

[LeonidVas]

Hi! As for me now the test seems not bad. I think the result could be something like this:

--- gh-34: Check that fiber is not blocked in the following case.
-- A connection conn is acquired from a pool and we acquire a
-- lock. Then we start the first fiber with pool:put(conn)`, which
-- try to acquire lock and yield. Then the second fiber is started
-- to perform request with `conn:execute()` which try to acquire
-- lock, because the connection is still marked as `usable` and
-- yield too. It's appeared that the lock is never released. Now,
-- it's fixed.
local function test_block_fiber_inf(test, pool)
    test:plan(2)
    local conn = pool:get()

    local function fiber_block_request(conn)
        local res, err = pcall(conn.execute, conn, 'SELECT "a"')
        test:ok(res == false and string.find(err, 'Connection is broken'),
            'query failed')
    end

    conn.queue:get()
    fiber.create(pool.put, pool, conn)
    local blocked_fiber = fiber.create(fiber_block_request, conn)
    conn.queue:put(true)

    local start_time = fiber.time()
    local timeout = 5
    local is_alive = true

    -- Wait for the blocked fiber to finish,
    -- but no more than "timeout" seconds.
    while fiber.time() - start_time < timeout and is_alive do
        fiber.sleep(0.1)
        is_alive = blocked_fiber:status() ~= 'dead'
    end

    test:ok(is_alive == false, 'fiber is not blocked')
end

See some comments below.

[romanhabibov]

Hi! As for me now the test seems not bad. I think the result could be something like this:

--- gh-34: Check that fiber is not blocked in the following case.
-- A connection conn is acquired from a pool and we acquire a
-- lock. Then we start the first fiber with pool:put(conn)`, which
-- try to acquire lock and yield. Then the second fiber is started
-- to perform request with `conn:execute()` which try to acquire
-- lock, because the connection is still marked as `usable` and
-- yield too. It's appeared that the lock is never released. Now,
-- it's fixed.
local function test_block_fiber_inf(test, pool)
    test:plan(2)
    local conn = pool:get()

    local function fiber_block_request(conn)
        local res, err = pcall(conn.execute, conn, 'SELECT "a"')
        test:ok(res == false and string.find(err, 'Connection is broken'),
            'query failed')
    end

    conn.queue:get()
    fiber.create(pool.put, pool, conn)
    local blocked_fiber = fiber.create(fiber_block_request, conn)
    conn.queue:put(true)

    local start_time = fiber.time()
    local timeout = 5
    local is_alive = true

    -- Wait for the blocked fiber to finish,
    -- but no more than "timeout" seconds.
    while fiber.time() - start_time < timeout and is_alive do
        fiber.sleep(0.1)
        is_alive = blocked_fiber:status() ~= 'dead'
    end

    test:ok(is_alive == false, 'fiber is not blocked')
end

See some comments below.

I'm okay with your test version. I used it.

[LeonidVas]

LGTM.

[Totktonada]

I have no objections aside of naming and the forgotten print. I looked at the code and didn't look at the test again (I did it the past time).

Please, rebase the patch upward master.

That's all. I think we can push after that.

[romanhabibov]

Rebased. Let's push?

[Totktonada]

Waiting for PR #58 to run tests.

[Totktonada]

Rebased on top of the latest master.