Add clock interface to make testing and process a lot easier

[pcman312]

No description provided.

[bluele]

Hi, @pcman312 .
Thank you for a good suggestion!
It seems that clockwork is good library, but I want to avoid increasing the dependency library as much as possible.
As an alternative,

// gcache
var Now = time.Now

// test, or other packages
func f() {
    gcache.Now = func() time.Time {
       return time.Now().Add(time.Hour)
    }
}

How about making customizable function like the above?

[pcman312]

That approach would definitely work, though it limits flexibility and forces the entire library to use a single Now function instead of making it specific to the cache instance.
What about making an interface around it such that you can use clockwork but without having the dependency?

type Clock interface {
  Now() time.Time
}

// Test within my app
fakeClock := clockwork.NewFakeClock()
cache := gcache.New(...).
  Clock(fakeClock).
  Build()
}

That way you have the flexibility of using clockwork (or any other library that has a Now() function) without pulling in the dependency or having it be a global function (or even a per-cache function). This also allows for future expansion to the clock interface if future functions are desired without having to re-architect another special function into the cache.

Thanks @bluele!

[bluele]

@pcman312
Thanks for your reply.
Certainly that method can avoid dependence.
However, in testing, it is necessary to change the Now function for each cache object operation instead of cache object unit.

cache.SetWithExpire("key", "value", 3*time.Second)
gcache.Now = func() time.Time {
    return time.Now().Add(3*time.Second)
}
// expects KeyNotFound error
_, err := cache.Get("key")

So, in our case, I think the exported Now function is easier to use.

[pcman312]

There are several serious problems with making a global Now function:

1. Race conditions
   If you have multiple go routines manipulating the function (or manipulating the internal workings of the function) simultaneously, you end up with a race condition.
2. Bleedover problems
   If you have testA change Now to funcA and then testB doesn't change Now to its own function, you end up with a test bleedover effect where the order of the tests running matters. If testA runs before testB it behaves differently compared with testB running before testA. This results in inconsistent behavior in unit tests. This behavior could also be exhibited in non-test code if any manipulation of the Now function is done at runtime.

Here are 4 possible solutions (from the context of unit tests):

Use global Now and system clock:

gcache.Now = func() time.Time {
	return time.Now().Add(3 * time.Second)
}

• This has the problem of time offsets because of the order of operations. This is much more apparent when the kernel pauses execution for some reason (such as switching what CPU the process is running on).
• Because of the time offset, placement of this call in relation to other related calls is paramount.
• Now is global, so you can't have different clocks per cache instance (see: race conditions, bleedovers)

Use global Now and fake clock:

fakeClock := clockwork.NewFakeClock()
gcache.Now = func() time.Time {
	return fakeClock.Now()
}

• This eliminates the time offset problem mentioned in the first option, but still has the race condition and bleedover problems

Use clockwork injected clock (with default to system clock):

fakeClock = clockwork.NewFakeClock()
gcache.New(size).
	Clock(fakeClock).
	Build()

• Eliminates the race condition and bleedover problems
• Doesn't have time offset problems associated with using the system clock
• Doesn't force the user to specify the clock since it defaults to using the system clock

Use fake clock that isn't clockwork:

type FakeClock struct {
	CurrentTime time.Time
}

func (fc FakeClock) Now() time.Time {
	return fc.CurrentTime
}
...
fakeClock := FakeClock{}
gcache.New(size).
	Clock(fakeClock).
	Build()

• Eliminates the race condition and bleedover problems, but doesn't force the user to use the clockwork library (or any other library).
• Minimal overhead of creating and using a FakeClock object

Injecting the clock into the cache forces the user to have atomic unit tests. Without that, unit tests may have flaky and inconsistent behavior. Production code can also have that flakiness if the feature is abused. Either approach can be abused, but injecting the clock limits the scope of that abuse and helps guide the user to a good pattern of behavior. It's also worth noting that injecting the clock into the cache is a minimal amount of extra work that is only done in unit tests since the clock defaults to the system clock while giving the user the control that they need during unit tests. Adding a fake clock is a small price to pay to gain atomicity and control over time in unit tests.

[erwanor]

Thank you for contributing.

Imho, in the interest of better testing and maintainability (in addition to the reasons above mentioned) it is better to go with a pluggable fake clock object on a per cache basis. We lack tests for the SetWithExpire that will make it convenient to formally test that feature (I will do it).

Heads up, you have included your vendor/ directory in your PR.

[pcman312]

Thanks @aaronwinter! I left everything as is since the start of the conversation so we could settle on how we wanted to handle this. I'll get the PR updated and let you guys know when it's ready.

[bluele]

Thank you very much for sorting out an issue.

Use fake clock that isn't clockwork:

type FakeClock struct {
	CurrentTime time.Time
}

func (fc FakeClock) Now() time.Time {
	return fc.CurrentTime
}
...
fakeClock := FakeClock{}
gcache.New(size).
	Clock(fakeClock).
	Build()

I think this solution is best.
This can eliminate the dependency and it is easy to handle in this any case.

What do you think of the definition like the following?

type Clock interface {
	func Now() time.Time
}

func (cb *CacheBuilder) Clock(clock Clock) *CacheBuilder

[pcman312]

@bluele That's basically what I had intended on building. I am closing this PR and opening another with those changes. I'll reference this PR in the description.