Store connection_handler per-fiber and per-thread.

[ioquatix]

Default to per-fiber if specified, otherwise revert to per-thread.

Summary

In Rails 6, the "per-thread connection handler" was made to be actually per-thread, not per-fiber. However, I'm concerned this breaks falcon which is a fiber-per-request web server. So, I'd like to explore how we can fix this.

One thing I also need to do is check if this breaks on falcon or not, so independently of this PR, I'll evaluate how the issue manifests (or not) on falcon.

#30047

[ioquatix]

I've added some extra notes on async-postgres because I need to review and add specs to check for overlapping transactions.

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[ioquatix]

This issue is still outstanding.

[rafaelfranca]

I considered this before.

I think we discussed about adding a configuration to allow people to chose where to put those variable, the default being per-fiber. It was only missing someone working on it.

I also don't think this is the only place that needs to change.

[ioquatix]

The change that was implemented made it per-thread with no configuration. Some might consider that a breaking change to a public interface. That being said, AR has a lot of per-thread state making it somewhat unsuitable for fiber based concurrency. Open to ideas/discussion around how we improve the situation.

[rafaelfranca]

Allowing configuration would be a good start. If we just change it will likely break some apps so I think we should keep the current behavior available for those apps but chose a path forward for new apps.

I know @matthewd has good opinions about this.

[ioquatix]

Allowing configuration would be a good start. If we just change it will likely break some apps so I think we should keep the current behavior available for those apps but chose a path forward for new apps.

It was already changed from fiber-local to thread-local: 5ce3e02 between rails 5 and rails 6.

I guess it doesn't matter that much since until recently there were very few cases where this would be an issue.

I think the best solution to this problem is to avoid per-thread and per-fiber state.

However, if you do need to have some hidden local state, the correct place to put it is per-fiber, IMHO.

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[ioquatix]

This issue is still outstanding.

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[ioquatix]

@tenderlove what do we do here?

[hakusaro]

@eileencodes heya hey! I know this issue has been around for a bit. I know it’s a bit rude to ask just out of the blue since we don’t know each other, but it seems like the benefit outweighs the risk here. With Ruby 3 just over the horizon, fibers will play a huge role in improving concurrency in the future. Do you mind taking a quick glance at this problem in context of the awesome recent multi-db changes?

[eileencodes]

Do you mind taking a quick glance at this problem in context of the awesome recent multi-db changes?

This PR needs tests to be mergeable and should be made to be configurable like Rafael requested. There is also only going to be one connection handler going forward (see #40370) so this might be less of an issue in that case.

[ioquatix]

Allowing configuration would be a good start. If we just change it will likely break some apps.

I agree, probably the entire connection pool needs to be configurable - either per thread or per fiber.