Return to previous date after block when in threadsafe mode

[tom-lord]

The thread_safe option introduced to Timecop v0.9.0 (and still present in v0.9.1) contains a major bug: The time is not automatically returned to its original state, when Timecop.freeze or Timecop.travel are called with a block.

The original PR for the feature actually worked fine, but was broken by this separate commit line, in this PR.

Minimal reproduction example (as seen from the test in this PR):

Timecop.thread_safe = true
date = Time.local(2017, 10, 8)

Timecop.freeze(date) { }
puts Time.now  # => 2017-10-08 00:00:00 +0100

This bug only occurs in threadsafe mode.

The issue stems from a 1-character mistake: using @stack instead of stack. The latter calls this method:

def stack
  if @thread_safe
    Thread.current[:timecop_stack] ||= []
    Thread.current[:timecop_stack]
  else
    @stack
  end
end

When thread_safe mode is enabled, @stack remains an empty array - therefore the original time was not being returned to.

[tom-lord]

Incidentally, something I'm a little unclear on here: Why is Timecop.thread_safe= an option? Is there a significant performance gain (or some other benefit) from running Timecop in non-threadsafe mode?

In other words, would it not make more sense to just make Timecop always threadsafe and deprecate the opt-in config option?

[lucidstack]

Gentle nudge towards getting this merged? I'm running exactly into this issue with timecop and parallel_tests 😕 (thank you for spotting the solution, @tom-lord!)

[lukasztackowiak]

@travisjeffery It'll be great if you can find some time to verify & merge this PR.

[jonekdahl]

I have also run into this exact problem when using timecop and parallel_tests together. If there is anything I can do to help getting this fix merged and released, please let me know.

[Aryk]

Incidentally, something I'm a little unclear on here: Why is Timecop.thread_safe= an option? Is there a significant performance gain (or some other benefit) from running Timecop in non-threadsafe mode?

In other words, would it not make more sense to just make Timecop always threadsafe and deprecate the opt-in config option?

Just curious if this question was answered. Seems like it should just be always threadsafe, no?

[dcoleman0711]

I just encountered this and opened a duplicate bug report before I saw this: #216

[Aryk]

Looking at the most recent shows this one character change is still not merged. Any chance it will get merged soon?

[tom-lord]

I'm not aware of any reason not to merge this; it's just been sat here pending for over 3 years 😅

@joshuacronemeyer , thoughts?

Deprecating thread_safe as an opt-in configuration also seems sensible to me, but that could be a separate PR.

[joshuacronemeyer]

I don't see why we shouldn't add this to 0.9.3. I do see the failed build for ruby 1.9.3 though and i can't seem to get these really old rubies running on my machine. I'll merge and hope the build passes.

[Aryk]

When is this change live on rubygems "timecop". Is it already?

[tom-lord]

@Aryk It would require a version bump + release

I've also just noticed this file: https://github.com/travisjeffery/timecop/blob/master/History.md

...so it would be good practice to make a note of changes like this in there before a release. (I guess I didn't notice it 3 years ago, as such a file is more commonly called CHANGELOG.md.)

Also, it seems there's one issue labelled as TODO before the next release (not sure how critical that is, though?) https://github.com/travisjeffery/timecop/milestone/3

[joshuacronemeyer]

I think that last issue should be an easy fix, so i'd like to get it in. I'll set a deadline for this weekend on the release so if no PR comes by the weekend we'll release without it. @tom-lord thanks for noticing the History file. If you have time to update it that would be appreciate and feel free to rename.