Code

Opened 3 years ago

Closed 5 months ago

#15806 closed Bug (fixed)

Error in filebased cache culling

Reported by: witek@… Owned by: Anssi Kääriäinen <akaariai@…>
Component: Core (Cache system) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In line 115 of filebased.py we have:

try:
    filelist = sorted(os.listdir(self._dir))
except (IOError, OSError):
    return

if self._cull_frequency == 0:
    doomed = filelist
else:
    doomed = [os.path.join(self._dir, k) for (i, k) in enumerate(filelist) if i % self._cull_frequency == 0]

In case of _cull_frequency == 0 instead of "doomed = filelist" we should have sth like:

doomed = [os.path.join(self._dir, d) for d in filelist]

Otherwise doomed tuple will not contains absolute paths but relative.

Attachments (3)

tests_for_15806.diff (2.4 KB) - added by desh 3 years ago.
Regression test for filebased cache with CULL_FREQUENCY=0
patch_for_15806.diff (566 bytes) - added by desh 3 years ago.
Initial patch
patch_with_tests_for_15806.diff (3.0 KB) - added by desh 3 years ago.
United diff (code + tests)

Download all attachments as: .zip

Change History (12)

comment:1 Changed 3 years ago by jacob

  • Easy pickings unset
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

See also #15825. Not exactly related, but right around in that same part of code and probably worth fixing at the same time.

comment:2 Changed 3 years ago by desh

  • Owner changed from nobody to desh
  • Status changed from new to assigned
  • UI/UX unset

Changed 3 years ago by desh

Regression test for filebased cache with CULL_FREQUENCY=0

Changed 3 years ago by desh

Initial patch

comment:3 Changed 3 years ago by desh

  • Has patch set

comment:4 follow-up: Changed 3 years ago by aaugustin

  • Patch needs improvement set

Could you consolidate the tests and the fix in the same patch?

comment:5 in reply to: ↑ 4 Changed 3 years ago by desh

Replying to aaugustin:

Could you consolidate the tests and the fix in the same patch?

Yep, sorry for that, just done tests before actually fixing so they came up first.

You can actually pick the merged_patch.diff from #15825, it contains all in one, tests and code for #15806 and #15825.

Changed 3 years ago by desh

United diff (code + tests)

comment:6 Changed 3 years ago by desh

  • Patch needs improvement unset

comment:7 Changed 3 years ago by desh

  • Owner desh deleted
  • Status changed from assigned to new

comment:8 Changed 7 months ago by marfire

  • Has patch unset
  • Version changed from 1.3 to master

This is fixed by the solution to #20536.

comment:9 Changed 5 months ago by Anssi Kääriäinen <akaariai@…>

  • Owner set to Anssi Kääriäinen <akaariai@…>
  • Resolution set to fixed
  • Status changed from new to closed

In 7be638390e18fcbfaaed638f9908673360c280d3:

Fixed #20536 -- rewrite of the file based cache backend

  • Safer for use in multiprocess environments
  • Better random culling
  • Cache files use less disk space
  • Safer delete behavior

Also fixed #15806, fixed #15825.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.