bokmann / business_time

Support for doing time math in business hours and days
MIT License
1.27k stars 215 forks source link

[Bug] Concurrency issues #209

Closed pmcnano closed 2 years ago

pmcnano commented 3 years ago

Noticed some issues, took me a while to investigate but found the culprit. If you run Puma with concurrency > 1, the config is only initialized for the first worker. When they fork, the config is lost on the extra workers.

Steps to reproduce:

Depending on the worker that is assigned to your request, you will get the correct date April 1st 2021 or the incorrect date ignoring the holiday May 31st 2021.

I had to fix this by running the initializer through puma's after_worker_boot hook. But the issue relies on how Threads are being used in the config.rb file. I don't have enough knowledge on Threads to fix it, I tried and failed, but I share this information so maybe it get's fixed.

pmcnano commented 3 years ago

Btw, now that I know this, I realize what happened back here: https://github.com/bokmann/business_time/issues/206 so I am closing it.

rmm5t commented 2 years ago

I'm a tad confused by this report for multiple reasons:

  1. Puma workers are processes, not threads.
  2. The correct date for your scenario is June 1, 2021, not April 1.
    >> BusinessTime::Config.holidays << '2021-05-31'.to_date
    => #<Set: {Mon, 31 May 2021}>
    >> 1.business_day.after('2021-05-28'.to_date)
    => Tue, 01 Jun 2021
  3. I cannot reproduce your scenario using puma workers, even if it is related to the puma worker boot. Are you sure you performed a full restart of the application during your testing?
pmcnano commented 2 years ago

Hey! I'll get back to you tomorrow, however: 1) Yes I am aware, but when they are forked the config is lost. 2) You are right, my bad on the correct date. 3) It's a hit or miss, depending on the process. I will re-test tomorrow and try to come up with something beter for testing.

rmm5t commented 2 years ago

Thanks. A simple app that reproduces the issue would be much appreciated. Either way, this sounds like an issue outside of this gem, but it might be helpful in a README.

pmcnano commented 2 years ago

@rmm5t well, this has been eye opening. I wasn't able to replicate in the new minimal test app and I figured out what was happening. I thank you for your time.

So, what was happening is, at some point we started using some experimental options, specifically, fork_worker is the culprit.

I'm closing this and sorry for the time spent.

rmm5t commented 2 years ago

@pmcnano No worries. Thanks for the follow-up.