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

Thread safety with regard to time zone handling #220

Closed
knu opened this Issue Nov 11, 2016 · 11 comments

Comments

Projects
None yet
2 participants
@knu

knu commented Nov 11, 2016

Rufus-scheduler introduced ZoTime in 3.1.0, and since then ENV['TZ'] may get modified in run time, where other threads may be creating Time objects or spawning child processes.

So, my question is, when do ZoTime#time and ZoTime#utc have a chance to be called? Is it only when a schedule is registered using a time zone in the cron line, or is there a chance to be re-evaluated when some event occurs?

@jmettraux

This comment has been minimized.

Show comment
Hide comment
@jmettraux

jmettraux Nov 11, 2016

Owner

Hello,

as you guessed ZoTime#time is called upon registering a cron schedule.

It is also called upon a CronJob#next_time or a CronJob#previous_time.

ENV['TZ'] is consulted by cron jobs when there is no definite time zone in the cron string (upon creation and for #next_time and #previous_time. That might be a problem (maybe that's what prompted you to open this issue). One solution could be to keep track of the timezone at the point of creation of the CronJob instance.

I'd be glad to help once I know exactly what bothers you.

Best regards.

Owner

jmettraux commented Nov 11, 2016

Hello,

as you guessed ZoTime#time is called upon registering a cron schedule.

It is also called upon a CronJob#next_time or a CronJob#previous_time.

ENV['TZ'] is consulted by cron jobs when there is no definite time zone in the cron string (upon creation and for #next_time and #previous_time. That might be a problem (maybe that's what prompted you to open this issue). One solution could be to keep track of the timezone at the point of creation of the CronJob instance.

I'd be glad to help once I know exactly what bothers you.

Best regards.

@knu

This comment has been minimized.

Show comment
Hide comment
@knu

knu Nov 11, 2016

Thanks for the quick reply!

I have a system that persistently runs a scheduler built with rufus-scheduler, and the system dynamically adds or deletes schedules upon a request via API. Each schedule periodically invokes a specified job on a specified time/interval, and the process involves running an external command, making an HTTP request and accessing DB while executing jobs.

So, what I need here is make sure jobs are not affected by registration of new schedules.

knu commented Nov 11, 2016

Thanks for the quick reply!

I have a system that persistently runs a scheduler built with rufus-scheduler, and the system dynamically adds or deletes schedules upon a request via API. Each schedule periodically invokes a specified job on a specified time/interval, and the process involves running an external command, making an HTTP request and accessing DB while executing jobs.

So, what I need here is make sure jobs are not affected by registration of new schedules.

@jmettraux jmettraux self-assigned this Nov 11, 2016

@jmettraux

This comment has been minimized.

Show comment
Hide comment
@jmettraux

jmettraux Nov 11, 2016

Owner

Ah, I understand. Indeed, the calls to ZoTime#in_zone are problematic. I will study how to avoid using it. It's currently used in two places in ZoTime.

Thanks for reporting that. Please give me a few days.

Owner

jmettraux commented Nov 11, 2016

Ah, I understand. Indeed, the calls to ZoTime#in_zone are problematic. I will study how to avoid using it. It's currently used in two places in ZoTime.

Thanks for reporting that. Please give me a few days.

@knu

This comment has been minimized.

Show comment
Hide comment
@knu

knu Nov 11, 2016

Great. Thanks for taking the time! Actually I was about to start working on a PR, but before that I just wanted to know what you'd think about the idea of fixing it.

knu commented Nov 11, 2016

Great. Thanks for taking the time! Actually I was about to start working on a PR, but before that I just wanted to know what you'd think about the idea of fixing it.

@jmettraux jmettraux removed their assignment Nov 11, 2016

@jmettraux

This comment has been minimized.

Show comment
Hide comment
@jmettraux

jmettraux Nov 11, 2016

Owner

OK, got it. I'd be glad to have a PR from you.

Owner

jmettraux commented Nov 11, 2016

OK, got it. I'd be glad to have a PR from you.

@knu

This comment has been minimized.

Show comment
Hide comment
@knu

knu Nov 11, 2016

After some investigation, I've found you need to add back the parser for TZ values. TZInfo or ActiveSupport::TimeZone can parse timezone representations like Asia/Tokyo or PST, but not JST-9. It is likely you could do that better. 😥

knu commented Nov 11, 2016

After some investigation, I've found you need to add back the parser for TZ values. TZInfo or ActiveSupport::TimeZone can parse timezone representations like Asia/Tokyo or PST, but not JST-9. It is likely you could do that better. 😥

@jmettraux jmettraux self-assigned this Nov 11, 2016

@jmettraux

This comment has been minimized.

Show comment
Hide comment
@jmettraux

jmettraux Nov 12, 2016

Owner

I will bring back the dependency on TZInfo and cook something based on it that doesn't touch ENV['TZ'].

Thanks again.

Owner

jmettraux commented Nov 12, 2016

I will bring back the dependency on TZInfo and cook something based on it that doesn't touch ENV['TZ'].

Thanks again.

jmettraux added a commit that referenced this issue Nov 12, 2016

jmettraux added a commit that referenced this issue Nov 12, 2016

jmettraux added a commit that referenced this issue Nov 13, 2016

jmettraux added a commit that referenced this issue Nov 13, 2016

jmettraux added a commit that referenced this issue Nov 13, 2016

jmettraux added a commit that referenced this issue Nov 13, 2016

jmettraux added a commit that referenced this issue Nov 13, 2016

jmettraux added a commit that referenced this issue Nov 13, 2016

jmettraux added a commit that referenced this issue Nov 13, 2016

jmettraux added a commit that referenced this issue Nov 13, 2016

jmettraux added a commit that referenced this issue Nov 13, 2016

jmettraux added a commit that referenced this issue Nov 14, 2016

jmettraux added a commit that referenced this issue Nov 14, 2016

jmettraux added a commit that referenced this issue Nov 14, 2016

@jmettraux

This comment has been minimized.

Show comment
Hide comment
@jmettraux

jmettraux Nov 14, 2016

Owner

It's taking its time but it's starting to look good.

Owner

jmettraux commented Nov 14, 2016

It's taking its time but it's starting to look good.

jmettraux added a commit that referenced this issue Nov 15, 2016

jmettraux added a commit that referenced this issue Nov 15, 2016

Store timezone instance in CronLine, gh-220
[ci skip]

Still, the "shortest delta" specs are 3 times slower than they were before this gh-220 rework. Digging...

jmettraux added a commit that referenced this issue Nov 17, 2016

Adapt CronLine spec to explict @Timezone, gh-220
Beforehand, the local timezone in the CronLine was stored as `nil`, from gh-220 on, it is stored as a TZInfo::Timezone (like CronLine with non-local timezones).

jmettraux added a commit that referenced this issue Nov 17, 2016

Complete adaptation of cronline_spec.rb to gh-220
Though it's running in ~19.5 seconds vs ~7.0 seconds (pre gh-220) on Ruby 2.2.5p319.

Make it run, make it fast.

jmettraux added a commit that referenced this issue Nov 18, 2016

jmettraux added a commit that referenced this issue Nov 18, 2016

jmettraux added a commit that referenced this issue Nov 18, 2016

jmettraux added a commit that referenced this issue Nov 18, 2016

jmettraux added a commit that referenced this issue Nov 19, 2016

jmettraux added a commit that referenced this issue Nov 21, 2016

jmettraux added a commit that referenced this issue Nov 21, 2016

Introduce ZoTime / Time #to_debug_s, gh-220
Focus on utc_offset instead of zone name (which various wildly between 1.8, 2.x or JRuby)
@jmettraux

This comment has been minimized.

Show comment
Hide comment
@jmettraux

jmettraux Nov 21, 2016

Owner

Hello, apart from a slight problem with JRuby, this transition is done.

If you have the time, please have a look 781c009...184bd6a

Your comments are welcome.

Many thanks for making me realize the problem behind my ENV['TZ'] approach.

I will close this issue soon. I'll release this as part of the upcoming 3.3.0

Owner

jmettraux commented Nov 21, 2016

Hello, apart from a slight problem with JRuby, this transition is done.

If you have the time, please have a look 781c009...184bd6a

Your comments are welcome.

Many thanks for making me realize the problem behind my ENV['TZ'] approach.

I will close this issue soon. I'll release this as part of the upcoming 3.3.0

@knu

This comment has been minimized.

Show comment
Hide comment
@knu

knu Nov 27, 2016

Wow, this must have been a tough work! Thanks!

I've deployed the current master to my personal installation and so far it's been working fine.

knu commented Nov 27, 2016

Wow, this must have been a tough work! Thanks!

I've deployed the current master to my personal installation and so far it's been working fine.

@jmettraux

This comment has been minimized.

Show comment
Hide comment
@jmettraux

jmettraux Nov 27, 2016

Owner

Thanks, closing for now !

Owner

jmettraux commented Nov 27, 2016

Thanks, closing for now !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment