rails / solid_queue

Database-backed Active Job backend
MIT License
1.84k stars 110 forks source link

Default value for `enqueue_after_transaction_commit` #212

Closed zarqman closed 5 months ago

zarqman commented 5 months ago

I'm wondering about the current default value of SolidQueue.enqueue_after_transaction_commit = true (as added in #209). I'd like to suggest that false would be a better default.

1. Risk of a future unexpected behavior change

On a DB-backed queue (SolidQueue, GoodJob, etc), Rails 7.1 and prior already behave as if this is false. (Assuming the queue and app both use the same DB, which I believe is the most common case, by far.)

By defaulting SolidQueue to true, apps switching from Rails 7.1 to Rails 7.2 will suddenly change behavior--without a deprecation warning or other heads up. This seems like a lurking gotcha for developers.

2. Rails docs encourage false

As the present time, the Rails edge docs say, "Active Job backends that use the same database as Active Record as a queue, should generally prevent the deferring, and others should allow it." (ref)

"Prevent the deferring" would be a default of false.

I suspect this encouragement is precisely because it avoids the sudden behavior change described in (1).

--

I believe that most small to medium sized apps will benefit from false all the time, especially as the default across all types of jobs. I'd expect wanting a global setting of true to be limited mostly to larger apps where a separate DB for SolidQueue is used.

However, if a default of true is really wanted long term, that transition should be made explicit. One way to do this would be with a major new release of SolidQueue after Rails 7.2 is released, such that developers would be drawn to look at a "breaking changes" section of the release notes where the behavior change would be documented.

Another approach would be to start writing deprecation warnings for a period of time before switching the default. Again, after the Rails 7.2 release so that apps can set their desired behavior before the default behavior is changed.

I emphasize the need to do any of this after the Rails 7.2 release because there is no setting in Rails 7.1 (apart from deliberately configuring SQ to use a separate DB connection) to accomplish the equivalent behavior to true.

rosa commented 5 months ago

@zarqman, those are great points, especially the second one. I hadn't seen the Rails Edge docs before I chose the default of true.

I agree with you. Let's change that back to false. If you'd like to contribute a PR to that, of course feel free to do so! Otherwise I'll get it done shortly.

rosa commented 5 months ago

Done!

zarqman commented 5 months ago

Thanks so much for the quick response and for your work on SolidQueue!

dhh commented 1 month ago

After further consideration, we're going to change this back to true, but make sure we communicate about this clearly in the SQ 1.0 release documentation. The window for false is simply too small. By default, Rails 8.0 will use sqlite and sq out of the box, and sq will have a separate sqlite file for that use case. So transactional guarantees can't be offered in that case.

But worse, if you do start with mysql/pgsql, you might end up building app logic dependent on the transactional guarantees that lead to a lot of subtle bugs when scale necessitates that you move sq to its own db. That seems like a ticking bomb that's not worth the payoff.

I still like having it there as a very sharp knife for those who know exactly what they're doing. But it shouldn't be the default and it shouldn't be encouraged. We'll change both the Rails guide and the sq docs to reflect this.

rosa commented 1 month ago

We'll change both the Rails guide and the sq docs to reflect this.

The Rails guide change is in https://github.com/rails/rails/pull/52659. The change in Solid Queue (the default + docs) will be added for v1.0.