Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to disable bulkheads in a given thread #562

Merged
merged 1 commit into from
Nov 4, 2024
Merged

Conversation

kirs
Copy link
Contributor

@kirs kirs commented Oct 29, 2024

The default Semian's bulkhead implementation being not fiber safe makes it hard to experiment with concurrency in a larger app which relies on that implementation.

I'd like to introduce Semian.disable_bulkheads_for_thread which will let me experiment with async IO in an app that's using Semian's non-fiber-safe bulkheads.

Example:

Semian.disable_bulkheads_for_thread(Thread.current) do
  Async do
     # current thread has bulkheads disabled and can use ActiveRecord/Trilogy
  end
end

Without something like this, the 2nd fiber that tries to hit Activerecord fails with:

ActiveRecord::ConnectionAdapters::TrilogyAdapter::ResourceBusyError: [mysql_shard_0] timed out waiting for resource '1000mysql_shard_0'

@casperisfine
Copy link
Contributor

Have you seen #560 ?

@kirs
Copy link
Contributor Author

kirs commented Oct 30, 2024

Yes. From what I understand, that patch is helpful if you implement a custom async-friendly Semian adapter like in @kovyrin's example.

That's the best path; however it's worth implementing a custom adapter only when you know that async is going to stay in the app for the long term.

Meanwhile, I see benefits of having a shortcut like disable_bulkheads_for_thread to try async in a monolith app that's using Semian's default adapter.

thread.thread_variable_get(THREAD_BULKHEAD_DISABLED_VAR)
end

def disable_bulkheads_for_thread(thread)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you expect threads other than current to be passed in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally don't. I've decided on that API to make it clear to the user that it disables it on a specific thread, not all threads.

I could also make it disable_bulkheads_for_current_thread but that seemed more verbose.

Copy link
Contributor

@kovyrin kovyrin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this 👍🏻

@casperisfine
Copy link
Contributor

I don't know what to think about this. The point of bulkheads is to ensure all your capacity won't be stuck on a single slow resource.

So it seems to me what you want to do is to only acquire a single ticket for all your fibers, not just disable bulkheads.

But it's been a long time since I worked on all this, so I don't have any string opinion.

@kirs
Copy link
Contributor Author

kirs commented Nov 4, 2024

This is not a proper solution, it's more of an escape hatch to disable bulkheads to experiment with concurrency in an app that's using non-thread-safe bulkheads feature of Semian.

Bulkheads should be disabled (pass bulkhead: false) in a threaded environment (e.g. Puma or Sidekiq)

Once we run those experiments, we could work on proper thread and fiber-safe bulkheading.

@kirs kirs merged commit 4098e20 into main Nov 4, 2024
42 checks passed
@kirs kirs deleted the bulkheads-thread branch November 4, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants