Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add counter to cache_id #1797

Merged

Conversation

stillwaiting
Copy link
Contributor

Current (in master) version of generate_cache_id is collision-prone: for complex factories with multiple
uploaders we have quite a few flaky specs because of that. Having a counter, this and similar situations, like mentioned here, should be resolved completely.

@@ -178,7 +189,9 @@ def workfile_path(for_file=original_filename)
alias_method :full_original_filename, :original_filename

def cache_id=(cache_id)
raise CarrierWave::InvalidParameter, "invalid cache id" unless cache_id =~ /\A[\d]+\-[\d]+\-[\d]{4}\z/
# Earlier cache_id contained only 3 pieces, the "counter" part was introduced later.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to talk about later. Change to "Earlier version used 3 part cache_id."


class CacheCounter
@@counter = 0
def self.increment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please insert a linebreak before this method definition.

Old version of generate_cache_id was collision-prone: for complex factories with multiple uploaders we had quite a few flaky specs because of that. With a counter, this (and similar situations like carrierwaveuploader#1232) should be resolved completely.
@thomasfedb
Copy link
Contributor

@bensie can I get your thoughts on this?

@thomasfedb
Copy link
Contributor

CI is failing on JRuby and Rails master, both of which are expected.

@stillwaiting
Copy link
Contributor Author

Seems they are failing for quite a while, e.g. here I can see the same picture: https://travis-ci.org/carrierwaveuploader/carrierwave/builds/96401284

@thomasfedb
Copy link
Contributor

@stillwaiting yes, that was what I mean, that those cases also fail on master and the issue is not introduced by your changes.

@thomasfedb
Copy link
Contributor

GTTM @bensie

@mehlah
Copy link
Member

mehlah commented Dec 30, 2015

@thomasfedb @stillwaiting The build should be 💚 now thanks to #1807.
I reviewed the changes, and it looks good to me 👍

thomasfedb added a commit that referenced this pull request Dec 30, 2015
Add a counter to generated cache_id values to avoid collisions
@thomasfedb thomasfedb merged commit bcbedf0 into carrierwaveuploader:master Dec 30, 2015
@mtsmfm
Copy link

mtsmfm commented Jan 24, 2016

@thomasfedb How about backporting?
Our tests will fail if collision occurs 😢
Many carrierwave users may tackle this random failure.

@thomasfedb
Copy link
Contributor

@mtsmfm I'm very happy to consider back porting this to the 0.10-stable branch, although we need to get the CI build fixed first.

@mtsmfm
Copy link

mtsmfm commented Jan 25, 2016

OK, I'll investigate that 😉

@mehlah
Copy link
Member

mehlah commented Jan 27, 2016

@mtsmfm should be good with #1847 cc @thomasfedb.
We can start backporting some fixes for an intermediate release.

@mtsmfm
Copy link

mtsmfm commented Jan 27, 2016

@mehlah Thanks for letting me know 😄

mtsmfm pushed a commit to mtsmfm/carrierwave that referenced this pull request Feb 2, 2016
------------------

Add counter to cache_id

Old version of generate_cache_id was collision-prone: for complex factories with multiple uploaders we had quite a few flaky specs because of that. With a counter, this (and similar situations like carrierwaveuploader#1232) should be resolved completely.
mtsmfm pushed a commit to mtsmfm/carrierwave that referenced this pull request Feb 3, 2016
------------------

Add counter to cache_id

Old version of generate_cache_id was collision-prone: for complex factories with multiple uploaders we had quite a few flaky specs because of that. With a counter, this (and similar situations like carrierwaveuploader#1232) should be resolved completely.
thomasfedb added a commit that referenced this pull request Feb 3, 2016
Backport cache counter_id (#1797) to 0.11-stable
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants