Clear active ActiveRecord connections between jobs.

[thomasfedb]

After having significant issues with long running database connections that only ever occurred on my worker processes, and not on my web processes, I investigated and found that Rails clears out active connections between each web request.

This PR implements the same connection clearing for Que. I'm sure the implementation is not ideal, and I'm happy to revise it with some advice. This has been running successfully in a production environment for a number of months, resolving all issues with long running database connections.

[chanks]

After skimming ActiveRecord's source, it looks like clear_active_connections! simply returns the current thread's connections to the pool. We use AR's with_connection method, which should handle this, though (and the with_connection block approach makes a lot more sense to me as a connection management mechanism - I'm not sure why Rails would even offer another way to do it). So my thinking is that either there's some flaw with Rails' connection pool implementation that would cause with_connection to not check in connections under some situations, or your app code is doing something unusual with AR connections somewhere but not always checking them back in.

The interface between Que and AR's connection pool is pretty simple, we just call with_connection and expect it to do the right thing. And since I don't remember getting any other complaints about this, I'm inclined to believe the problem isn't in Que, but I might be wrong though 😄 If you wrote some more about exactly what problems you saw with long-running connections, maybe it would sound familiar to someone else?

[thomasfedb]

Hey @chanks,

Thanks for the reply. I think I should note that in my workers I access multiple databases (the PostgreSQL one used by Que, as well as a SQL Server database), and using clear_active_connections! appears to ensure that all connections get returned, whilst with_connection doesn't seems to take care of the SQL Server connection.

I spent a long time debugging this issue with the SQL Server adapter developer: rails-sqlserver/activerecord-sqlserver-adapter#402

The fact that Rails internally uses clear_active_connections! between requests suggests to me that this is a good way to ensure that each request gets a clean slate for connections.

[chanks]

You're using ActiveRecord to access two different databases simultaneously? How are you managing that? Does ActiveRecord have some built-in facility for it or are you using another library in conjunction with it?

I'm skeptical that clear_active_connections! is even a reasonable API for managing connections in Ruby, the reentrant block approach supported by with_connection is cleaner and safer (in the sense of being much less prone to leaking connections, which is what's affecting you here). If you're using connections to a different database in a threaded environment, I think your code needs to ensure that those connections are cleaned up reliably. This means using with_connection (or its equivalent in whatever library you're using) to check out those SQL Server connections in your own code.

[joevandyk]

AR supports using a different connection in each model. http://pivotallabs.com/using-activerecord-with-multiple-databases/

[chanks]

That's interesting. I don't like it as much as Sequel's approach of having a Database class, but good to know.

[thomasfedb]

Hey @chanks,

AR natively supports multiple databases, and has for quite a while.

Given that you provide an AR integration, it seems reasonable to me that your integration should ensure connections are returned to the pool between jobs. AR transparently handles the multiple connections (eg: during association loading), and so a developer may not even know that an additional connection has been opened to another database.

As I mentioned before, clear_active_connections! is the API that Rails uses to ensure connections are returned to the pool between requests. I don't think it's unreasonable to suggest Que should do the same given that the current implementation (whilst perhaps theoretically cleaner) is inferior and mishandles cases when AR has used connections to multiple databases.

It's causes me significant grief, took over a month to debug and identify the root cause, and I would like to help others avoid the pitfall.

Cheers,
Thomas

[chanks]

Alright, you've convinced me. Sorry to be cranky, it just feels like all the time I'm able to devote to this gem goes right into working around problems with Rails/ActiveRecord.

Try current master and let me know how it works!

[thomasfedb]

Hey @chanks,

Cheers for fixing this, it's very much appreciated. I'm sorry that supporting Rails/AR is burdensome. I'll deploy your new version into our staging environment tomorrow.

Que is a fantastic project, and I chose it over all the other Rails worker gems as it is much easier for me to use (don't need a Redis DB, for example), works well, and was highly recommended.

Let me know if I can buy you a coffee or if you have a bitcoin address. 👍

[chanks]

Ha, no, thanks for the offer, though!

On Sat, Jul 25, 2015 at 11:58 AM, Thomas Drake-Brockman <
notifications@github.com> wrote:

Hey @chanks https://github.com/chanks,

Cheers for fixing this, it's very much appreciated. I'm sorry that
supporting Rails/AR is burdensome. I'll deploy your new version into our
staging environment tomorrow.

Que is a fantastic project, and I chose it over all the other Rails worker
gems as it is much easier for me to use (don't need a Redis DB, for
example), works well, and was highly recommended.

Let me know if I can buy you a coffee or if you have a bitcoin address. [image:
👍]

—
Reply to this email directly or view it on GitHub
#116 (comment).

[timanovsky]

Hello, this commit a2a46fb breaks rspec tests in our project. Had to rollback to 0.10

[chanks]

Breaks specs how? Are you calling Que::Job.work in them?

[timanovsky]

yes. And this somehow clears all (not que_jobs) records created by rspec/code under test

[joevandyk]

Hm, dropping the connection would roll back the transaction, and I think by
default rails/rspec runs the entire test in a single transaction. I could
see how we wouldn't want to clear the connection between jobs in tests.

Joe

On Fri, Sep 25, 2015 at 12:31 PM, Alexey Timanovsky <
notifications@github.com> wrote:

yes. And this somehow clears all (not que_jobs) records created by
rspec/code under test

—
Reply to this email directly or view it on GitHub
#116 (comment).

[chanks]

Well, it clears the active connections, which should roll back the current transaction, which wouldn't be helpful if you're wrapping each spec in a transaction.

I think you probably shouldn't be using Que::Job.work in your tests, if you want to test that a job works you should be using Que.mode = :sync or manually calling run on an instance of the job when you're unit testing the job.

[timanovsky]

Que::Job.work is used in main application code, not in tests. Tests just invoke it

[chanks]

Why are you calling it manually?

[timanovsky]

Well, we have many queues and each time we decide from which queue to take a job. This is application logic.

[chanks]

So you've implemented your own worker, and you have tests to make sure it's working jobs correctly, but these tests also are writing other records to the DB for some reason?

I don't know enough about what you're doing to know what the best solution for you is, but since Que::Job.work is not really intended to have a lot of DB state going on when it's called, I think it's reasonable for the clear_active_connections! call to happen there.

[timanovsky]

So you've implemented your own worker, and you have tests to make sure it's working jobs correctly, but these tests also are writing other records to the DB for some reason?

Not exactly. que Job run and code 'around' Que::Job.work does a lot of work using Rails. Tests themselves do not create any records.

Que::Job.work is not really intended to have a lot of DB state going on when it's called,

This is unfortunate, as it makes pretty serious assumptions about application. Would it be possible to provide an option or a hook to disable this feature?

[chanks]

Que::Job.work isn't usually called by the application, it's called by a worker, which is generally operating in a very simple loop without a lot of state going on, especially not in the DB (it's not intended to be called inside a transaction, for example). If you really wanted to call it from within your app, I suppose you could, but you'd need to be careful.

[timanovsky]

Yes, I understand all that. It worked perfectly before. Just design when one queue = one CPU process is not flexible enough. I would like to have a generic queue which I can pop from when I need.

[timanovsky]

May be you can move Que.adapter.cleanup! into worker thread (Que::Worker. work_loop) from Que::Job.work ? This will make abstraction more clear

Best regards,
Alexey Timanovsky.

On 25 September 2015 at 23:07, Chris Hanks notifications@github.com wrote:

Que::Job.work isn't usually called by the application, it's called by a
worker, which is generally operating in a very simple loop without a lot of
state going on, especially not in the DB (it's not intended to be called
inside a transaction, for example). If you really wanted to call it from
within your app, I suppose you could, but you'd need to be careful.

—
Reply to this email directly or view it on GitHub
#116 (comment).

[chanks]

That might be reasonable, I'm not sure. The next time I work on Que (maybe next Friday) I'll look into it. I'll also create a new issue so I don't forget.

In the meantime if you want to try moving it there and seeing if you run into any issues, I'd be curious to hear how it works out.