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

Reset corrupted PStore (fixes #409) #418

Merged
merged 2 commits into from Apr 14, 2014

Conversation

Projects
None yet
2 participants
@ddfreyne
Member

ddfreyne commented Apr 12, 2014

This PR makes a store reset itself when corruption is detected. This is fine, because the stores only contain data to speed up site compilation.

Fix for #409.

@ddfreyne ddfreyne added the bug/fix label Apr 12, 2014

@ddfreyne ddfreyne added the to-review label Apr 13, 2014

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne
Member

ddfreyne commented Apr 13, 2014

@ddfreyne ddfreyne changed the title from Reset corrupted PStore to Reset corrupted PStore (fixes #409) Apr 13, 2014

@loaded = true
rescue
FileUtils.rm_f(filename)
load

This comment has been minimized.

@bobthecow

bobthecow Apr 13, 2014

Member

i don't like an this rescue, since we're calling #load again inside the handler. anything that fails which isn't fixable by removing the pstore will just go into an infinite loop. Can we rescue a specific error?

@bobthecow

bobthecow Apr 13, 2014

Member

i don't like an this rescue, since we're calling #load again inside the handler. anything that fails which isn't fixable by removing the pstore will just go into an infinite loop. Can we rescue a specific error?

This comment has been minimized.

@ddfreyne

ddfreyne Apr 13, 2014

Member

Good point.

We can rescue TypeError, but to be 100% safe, the #load method should probably take an argument that indicates whether or not this invocation is a retry, and if it is, do not retry but just reraise the error. What do you think?

@ddfreyne

ddfreyne Apr 13, 2014

Member

Good point.

We can rescue TypeError, but to be 100% safe, the #load method should probably take an argument that indicates whether or not this invocation is a retry, and if it is, do not retry but just reraise the error. What do you think?

This comment has been minimized.

@bobthecow

bobthecow Apr 13, 2014

Member

yep. that works too.

@bobthecow

bobthecow Apr 13, 2014

Member

yep. that works too.

This comment has been minimized.

@ddfreyne

ddfreyne Apr 13, 2014

Member

Hmm, it’s actually not easy at all to make this fail (I noticed while trying to write a test for this). The only thing where this double-call check is necessary is when FileUtils.rm_f(filename) fails, because in the next call, the function will exit before the begin/rescue/end block, because !File.file?(filename).

So I think it’s OK to stick with the original implementation.

@ddfreyne

ddfreyne Apr 13, 2014

Member

Hmm, it’s actually not easy at all to make this fail (I noticed while trying to write a test for this). The only thing where this double-call check is necessary is when FileUtils.rm_f(filename) fails, because in the next call, the function will exit before the begin/rescue/end block, because !File.file?(filename).

So I think it’s OK to stick with the original implementation.

ddfreyne added a commit that referenced this pull request Apr 14, 2014

@ddfreyne ddfreyne merged commit afc8434 into release-3.6.x Apr 14, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@ddfreyne ddfreyne deleted the fix/reset-broken-pstore branch Apr 14, 2014

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Apr 14, 2014

Member

Merged anyway, but you can always give more feedback later :)

Member

ddfreyne commented Apr 14, 2014

Merged anyway, but you can always give more feedback later :)

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