Calling connect() after keepalive() about pgmoon HOT 16 CLOSED

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

from pgmoon.

Here's the culprit
c841be9#diff-ecfebe05627fcb262bfc1b60e5ea0306

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

from pgmoon.

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.

from pgmoon.

Side effects worse than a nil reference by default? 😏

from pgmoon.

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

from pgmoon.

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.

from pgmoon.

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

from pgmoon.

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

from pgmoon.

@ttfkam Works well enough for me. 👍

from pgmoon.

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.

from pgmoon.

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

from pgmoon.

Associating the pull request with this issue. #46

from pgmoon.

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

from pgmoon.

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.

from pgmoon.

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

from pgmoon.

Yes, #46

from pgmoon.