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

Setup a better busy_handler that respects a timeout #3

Merged
merged 2 commits into from Dec 6, 2023

Conversation

fractaledmind
Copy link
Owner

@fractaledmind fractaledmind commented Dec 6, 2023

As discussed in this PR rails/rails#49352, the new retries option in Rails isn't optimal.

  1. It is difficult, if not impossible, to determine what the correct limit is
  2. It can be slow in a multi-thread environment, as you will execute the Ruby busy_handler proc many, many, many times from within a C control frames

This provides a superior alternative which still respects a timeout, but it allows for other threads/fibers to take control while the current context is blocked on a write lock.

@fractaledmind fractaledmind merged commit fba8c88 into main Dec 6, 2023
1 check passed
@fractaledmind fractaledmind deleted the fractaledmind-patch-1 branch December 6, 2023 13:44
@suwyn
Copy link

suwyn commented Dec 6, 2023

@fractaledmind it would be interesting to benchmark a couple different busy handlers and choose the optimal one. My hunch is that we can gain some speed by losing the precision that calling Process.clock_gettime(Process::CLOCK_MONOTONIC) would give us.

Here is the one we use (using a lower retry_interval we've found it to be just as fast(if not a bit faster) as the previous RETRY_COUNT implementation in this gem):

retry_interval = 1.0e-12

if @config[:timeout]
  # although it appears our busy handler takes precedent over the sqlite handler for
  # busy_timeout we explicitly disable that to be sure
  @connection.busy_timeout(0)

  timeout_seconds = @config[:timeout] / 1000.0
  @connection.busy_handler do |count|
    if (count * retry_interval) > timeout_seconds
      false
    else
      sleep(retry_interval)
      true
    end
  end
end

@fractaledmind
Copy link
Owner Author

I am all for benchmarking and finding the optimal solution given the constraints we have.

@fractaledmind
Copy link
Owner Author

@suwyn
Copy link

suwyn commented Dec 13, 2023

Looks good @fractaledmind

Did you get a chance to do any benchmarking of the various routines?

@fractaledmind
Copy link
Owner Author

@suwyn I haven't tested anything beyond the previous one and this one, and even then only slightly. This new one was better, but I don't even have the run data to share. I want to do a proper experiment soon (i.e. in the new year, as I have to get thru RubyConf Taiwan first) where I try different retry intervals as well as different approaches altogether.

However, after a quick sanity check, I was confident this approach was better than the previous, and I didn't want to hold a release up because I was unable to do more through benchmarking. But I will!

@suwyn
Copy link

suwyn commented Dec 13, 2023

@fractaledmind sounds good, I'll try to find some time in the next couple weeks to do some testing as well. Safe travels!

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.

None yet

2 participants