Calling connect() after keepalive()

[gnois]

This used to work in 1.6 but no longer in 1.7 or 1.8, with error .../pgmoon/init.lua:194: attempt to index field 'sock' (a nil value)

File app/db.lua

local pg = require('pgmoon')
return pg.new({...})

File app/handler.lua

local db = require('app.db')
function ware1(db)
    db:connect()
    -- do work
    db:keepalive()
end

function ware2(db)
    db:connect()
    -- do work
    db:keepalive()
end

function handler(db)
    ware1(db)
    -- do some work
    ware2(db)
    -- do more work
end

I found that the self.sock handle is set to nil after keepalive(). Can we call connect() after keepalive() if they are within the same request?

Thanks!

[ttfkam]

I'm seeing this as well when running the build tests.

[ttfkam]

Here's the culprit https://github.com/leafo/pgmoon/commit/c841be95d9e2ce346b1311a91405343d7b1c4d48#diff-ecfebe05627fcb262bfc1b60e5ea0306

I'm assuming it was an oversight since I don't see any extra logic at work.

[thibaultcha]

It is an OpenResty paradigm to create one TCP socket in such a new() method, and not in connect() as pgmoon was previously doing. See https://github.com/openresty/lua-resty-redis for such an example, among others.

Maybe pgmoon shouldn't free the socket in :setkeepalive()? There could be some side-effects that I am not aware of.

[ttfkam]

Side effects worse than a nil reference by default? :smirk:

[thibaultcha]

If you feel like this behavior is more desirable than the current one, feel free to propose a PR to this project.

[ttfkam]

Looking at lua-resty-redis as a reference, https://github.com/openresty/lua-resty-redis/blob/403d2982ec1dacaa758301c788754a82c82b00d8/lib/resty/redis.lua#L99

it doesn't appear that the socket should be set to nil. I'll provide a PR soon.

[gnois]

I found the last paragraph of https://github.com/openresty/lua-resty-redis#set_keepalive gives a pointer that connect is allowed:

Any subsequent operations other than connect() on the current object will return the closed error.

Thanks @ttfkam and @thibaultcha

[ttfkam]

On the bright side, it turns out the failing tests were due to my build environment. The rockspec file really should have more dependencies defined.

On the downside, there are no tests for keepalive I can see. This probably makes sense since the connection behavior is different for OpenResty environments than for straight Lua usage.

I use pg_bouncer for my connection pooling, so I don't have a setup handy for it. @gnois, if you checkout my keepalive branch, I'd love to know that it works for you. The only two files you'd need to swap out are pgmoon/init.moon and pgmoon/init.lua

[gnois]

@ttfkam Works well enough for me. :+1:

[thibaultcha]

The rockspec file really should have more dependencies defined.

The rockspec is for production installations only, since the format does not support development-only dependencies yet. Development dependencies are to be manually installed; library maintainers have different ways of dealing with them and/or simply document them in the README. Feel free to send a PR improving the process for this library if you feel a need for it.

[ttfkam]

Still getting used to Lua. Thank you for the explanation.

[ttfkam]

Associating the pull request with this issue. #46

[yzk0281]

@ttfkam How did this problem solved in the end ? I came with the same problem.

[ttfkam]

I put up a pull request months ago with what I saw as the best solution at the time, but it's open source; you can't force anyone to actually accept the patch if they don't want to. Until @leafo accepts the patch or fixes on his own as he sees fit, you'll have to simply stop using pgmoon's implementation of keepalive().

As a workaround, you can use pgbouncer to manage connection pooling rather than relying on the database driver to do so.

[yzk0281]

@ttfkam Hi Sir, is Your PR change the keepalive function to not set sock = nil?

[ttfkam]

Yes, #46