Pardino::Cache::Store::Memory can not cache forever #827

Closed
snowyu opened this Issue Apr 10, 2012 · 14 comments

Comments

Projects
None yet
5 participants
@snowyu

snowyu commented Apr 10, 2012

Production:

cache = Padrino::Cache::Store::Memory.new(500)
# cache the key forever
cache.set(key, value)

# hit the cache
cache.get(key)

# we hope hit in the cache, but miss in the cache
cache.get(key)

the reason:

        # set(key, value, :expires_in=>-1)
        def set(key, value, opts = nil)
          delete(key) if @index.key?(key)
          if opts && opts[:expires_in]
            expires_in = opts[:expires_in].to_i
            # bug is here maybe the expires_in is -1:
            #expires_in = Time.new.to_i + expires_in if expires_in < EXPIRES_EDGE
            # fixed it:
            expires_in = Time.new.to_i + expires_in if expires_in >= 0 and expires_in < EXPIRES_EDGE
          else
            # -1 means forever
            expires_in = -1
          end
          ...
@snowyu

This comment has been minimized.

Show comment
Hide comment
@snowyu

snowyu Apr 10, 2012

Pardino::Cache::Store::File is the same problem set(key, value, :expires_in => -1)

snowyu commented Apr 10, 2012

Pardino::Cache::Store::File is the same problem set(key, value, :expires_in => -1)

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Apr 27, 2012

Member

mmm @nesquena I didn't remember the reason of EXPIRES_EDGE?

@snowyu you can easily change the behavior o this:

Padrino::Cache::Store::EXPIRES_EDGE = 999999999999999999
Member

DAddYE commented Apr 27, 2012

mmm @nesquena I didn't remember the reason of EXPIRES_EDGE?

@snowyu you can easily change the behavior o this:

Padrino::Cache::Store::EXPIRES_EDGE = 999999999999999999
@snowyu

This comment has been minimized.

Show comment
Hide comment
@snowyu

snowyu Apr 27, 2012

It's helpless here.

the "-1" means cache forever(never expires).

when first set cache:

cache.set(key, value)
  1. It will set the opts[:expires_in] = -1, it should be ok.
  2. we call the cache.get(key) first, the "cache.set(key, value, :expires_in => -1)" will be called too.
    So, the expires_in time will be set to "Time.new.to_i + expires_in"
  3. we call the cache.get(key) again, this key is already expired, so the missing occur.

snowyu commented Apr 27, 2012

It's helpless here.

the "-1" means cache forever(never expires).

when first set cache:

cache.set(key, value)
  1. It will set the opts[:expires_in] = -1, it should be ok.
  2. we call the cache.get(key) first, the "cache.set(key, value, :expires_in => -1)" will be called too.
    So, the expires_in time will be set to "Time.new.to_i + expires_in"
  3. we call the cache.get(key) again, this key is already expired, so the missing occur.
@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Jan 9, 2013

Contributor

Hey @snowyu would you mind trying out fix-forever-caching branch? It should be sorted now (see the test).

@DAddYE @nesquena I added that check for all the stores because I reckon it will make them more consistant. What do you think about this? We should update the docs, there's nothing referencing that in there. I'll hold from submitting the PR until somebody else checks this :)

Also, it seems as if the comment "Default expiry is 86400." is wrong since the default behaviour is to cache forever?.. 86400 is EXPIRES_EDGE.

Contributor

dariocravero commented Jan 9, 2013

Hey @snowyu would you mind trying out fix-forever-caching branch? It should be sorted now (see the test).

@DAddYE @nesquena I added that check for all the stores because I reckon it will make them more consistant. What do you think about this? We should update the docs, there's nothing referencing that in there. I'll hold from submitting the PR until somebody else checks this :)

Also, it seems as if the comment "Default expiry is 86400." is wrong since the default behaviour is to cache forever?.. 86400 is EXPIRES_EDGE.

@snowyu

This comment has been minimized.

Show comment
Hide comment
@snowyu

snowyu Jan 10, 2013

I've read it. That should be ok. But just be careful for other negative value: it means expired immediately. pls update document too. BTW, why not put this code to the abstract store class?

snowyu commented Jan 10, 2013

I've read it. That should be ok. But just be careful for other negative value: it means expired immediately. pls update document too. BTW, why not put this code to the abstract store class?

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Jan 10, 2013

Contributor

But just be careful for other negative value: it means expired immediately.

What do you mean by that? Is it for Memcached, Mongo & Redis? Or what? Thanks!

Contributor

dariocravero commented Jan 10, 2013

But just be careful for other negative value: it means expired immediately.

What do you mean by that? Is it for Memcached, Mongo & Redis? Or what? Thanks!

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Jan 10, 2013

Member

I think there is a confusion in variable naming.

An options key says expires_in and a variable says expires_in but they mean different entities.

The key should be names expires_in as it states a period of time.
The variable should be named expires_at because it points to a Time.

After such renaming the logic becomes more clear:
-1 is never put into the key and the variable can have -1 for infinite Time point.

Member

ujifgc commented Jan 10, 2013

I think there is a confusion in variable naming.

An options key says expires_in and a variable says expires_in but they mean different entities.

The key should be names expires_in as it states a period of time.
The variable should be named expires_at because it points to a Time.

After such renaming the logic becomes more clear:
-1 is never put into the key and the variable can have -1 for infinite Time point.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Jan 10, 2013

Contributor

Makes sense. Will draft something and post it back soon. Thanks again!

Contributor

dariocravero commented Jan 10, 2013

Makes sense. Will draft something and post it back soon. Thanks again!

@ghost ghost assigned dariocravero Jan 10, 2013

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Jan 31, 2013

Member

I agree with @ujifgc, thus we should also point EXPIRES_EDGE to -1 as for forever.

Member

DAddYE commented Jan 31, 2013

I agree with @ujifgc, thus we should also point EXPIRES_EDGE to -1 as for forever.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 10, 2013

Member

Should we do this before 0.11.0? I'd like to release soon, is this something we should aim to include? @DAddYE @dariocravero

Member

nesquena commented Mar 10, 2013

Should we do this before 0.11.0? I'd like to release soon, is this something we should aim to include? @DAddYE @dariocravero

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Mar 10, 2013

Contributor

I reckon we do @nesquena since it's not behaving properly. If it's ok with you, I will finish a consistent implementation either today or tomorrow so it doesn't go any further.

Contributor

dariocravero commented Mar 10, 2013

I reckon we do @nesquena since it's not behaving properly. If it's ok with you, I will finish a consistent implementation either today or tomorrow so it doesn't go any further.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 10, 2013

Member

OK, that'd be awesome. Just trying to sort tickets. Everyone feel free to move things out (or into) 0.11.0 depending on what we think should go into this release for sure. I would like to aim to release 0.11.0 by next Sunday latest if that's at all possible. I think we've let the release slip for too long now :)

Member

nesquena commented Mar 10, 2013

OK, that'd be awesome. Just trying to sort tickets. Everyone feel free to move things out (or into) 0.11.0 depending on what we think should go into this release for sure. I would like to aim to release 0.11.0 by next Sunday latest if that's at all possible. I think we've let the release slip for too long now :)

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Mar 10, 2013

Contributor

You're soo right! :) We will do that!..

Contributor

dariocravero commented Mar 10, 2013

You're soo right! :) We will do that!..

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Mar 16, 2013

Contributor

Sorry I couldn't finish this before, I will be away until Sunday evening and don't think I will be able to work on it until then!.. Moving this to 0.11.1 so it doesn't block the current release.

Contributor

dariocravero commented Mar 16, 2013

Sorry I couldn't finish this before, I will be away until Sunday evening and don't think I will be able to work on it until then!.. Moving this to 0.11.1 so it doesn't block the current release.

@ghost ghost assigned ujifgc Jul 8, 2013

ujifgc added a commit that referenced this issue Jul 8, 2013

@ujifgc ujifgc closed this in 001b618 Jul 9, 2013

Ortuna added a commit that referenced this issue Jul 9, 2013

deni64k added a commit to deni64k/padrino-framework that referenced this issue Jul 16, 2013

deni64k added a commit to deni64k/padrino-framework that referenced this issue Jul 16, 2013

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