Skip to content
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

[MRG+1] FS cache storage misuses mtime for expiration #1875

Merged
merged 1 commit into from Mar 30, 2016

Conversation

@nanolab
Copy link
Contributor

@nanolab nanolab commented Mar 23, 2016

FS cache storage misuses mtime for expiration

It checks cache directory modification time, but have to check file modification time.
@Digenis
Copy link
Member

@Digenis Digenis commented Mar 23, 2016

What bug does this fix?
What modifications do you need to do in the directory
that happens to update its mtime?

@nanolab
Copy link
Contributor Author

@nanolab nanolab commented Mar 23, 2016

When it create cache all works fine.
Once cache expires it checks directory mtime (.scrapy\httpcache\buyincoins\7e\7e2351803c5e337e02bd649c198ec6846b4147a9) and downloads and create all files inside this directory again. But directory mtime stay unchanged.
And it downloads and downloads....
So it have to check files mtime not directory.
I'm on Windows 10, Python 2.7.11

@Digenis
Copy link
Member

@Digenis Digenis commented Mar 23, 2016

I see. Yes, it's a bug.
Can you rename the commit message to something shorter (50 chars or less)
and meaningful on the same time?
E.g. FS cache storage misuses mtime for expiration.

@nanolab
Copy link
Contributor Author

@nanolab nanolab commented Mar 23, 2016

Renamed.

@Digenis
Copy link
Member

@Digenis Digenis commented Mar 23, 2016

doesn't seem like you did

@nanolab nanolab changed the title Update httpcache.py FS cache storage misuses mtime for expiration Mar 23, 2016
@Digenis
Copy link
Member

@Digenis Digenis commented Mar 23, 2016

No, this is the PR title. It seems you are not familiar with git, just github.
Anyway, doesn't matter for now, let's see what the maintainers have to say.

The change seems ok.
It should also be applied to newer releases.

@nyov
Copy link
Contributor

@nyov nyov commented Mar 26, 2016

+1, looks like a safe change in any case.

This should be merged into master, though (and then maybe backported). But alas, changing the target branch in an open PR isn't possible - hopefully someone does manually merge it.

@nanolab, you can change the commit title by doing a git checkout patch-1; git commit --amend -m "FS cache storage misuses mtime for expiration" or the equivalent in whatever GUI, then git push --force origin patch-1

@kmike kmike changed the title FS cache storage misuses mtime for expiration [MRG+1] FS cache storage misuses mtime for expiration Mar 27, 2016
@kmike
Copy link
Member

@kmike kmike commented Mar 27, 2016

Thanks @nanolab for the fix and @nyov and @Digenis for the review! I think that's not a big deal to leave commit message as-is; changing it would be nice, but not required. A good point about 1.0 branch; we should cherry-pick it to 1.1 and to master after merging.

@redapple redapple merged commit 2664238 into scrapy:1.0 Mar 30, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@redapple
Copy link
Contributor

@redapple redapple commented Mar 30, 2016

Cherry-picked to 1.1 and master branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.