I found that putting multiple theme('imagecache', ..)'s on a page consistently causes one or more image generations to fail, logged as a misguiding "directory is not writable" error.

example: in a cck node's .tpl.php:

print theme('imagecache', 'HistoricalMarkerImageThumb', $field_bigimage[0]['filepath']);
print theme('imagecache', 'HistoricalMarkerImageMed', $field_bigimage[0]['filepath']);
print theme('imagecache', 'HistoricalMarkerImageLarge', $field_bigimage[0]['filepath']);

Of this, typically the first call produces ..Thumb and ..Large, but not ..Med. An error is logged for ..Med. Reload of this page works fine (b/c two are already pre-gen).

I believe this is caused by multiple processes (each concurrent browser request for img-src="..." on the page) trying to access the same tmp file. It appears that an attempt at handling this was put in place in imagecache.module:~104, the test for !is_file($tmpdestination), but that completely bypasses the entire image creation routine! And since $generated = TRUE is set in both cases (if/else), the module continues along its merry way to call the missing image, adding a fistful of errors to the log.

    First Recommendation:
  • Make the tmp file unique:
    change :80
    $tmpdestination = file_directory_temp() . str_replace(dirname($path), '', $path);
    to
    $tmpdestination = file_directory_temp() . '/' . $preset['presetid'] . str_replace(dirname($path) . '/', '', $path);
    which would make tempfile paths like /tmp/1origimagename.jpg, /tmp/2origimagename.jpg, /tmp/3origimagename.jpg
  • I opted for pathid over pathname for brevity but still unique
  • Second Recommendation:

  • :~104-107
      if (!is_file($tmpdestination)) {
        $generated = TRUE;
        file_copy($source, $tmpdestination);
    
  • Remove $generated = TRUE;. Add curly brace to close this conditional block. Replace "else" portion (and removed closing curly brace) of this conditional block in :130-132 with $generated = TRUE;.

Comments

dopry’s picture

Could you roll a patch? prepending the presetname to the tmpfile seems like good solution.

http://www.drupal.org/diffandpatch

texas-bronius’s picture

StatusFileSize
new3.12 KB

Right on. I was just reading up about mismatchin' patchin'. I got caught up in trying to figure out how to work out the diff's in filenames, b/c this creates a new file called "imagecache.module.bak" (your preserved orig file's name)
See the patch, attached... please let me know how to improve my process: this patch will create a .bak file. :|

texas-bronius’s picture

n00b here.. I think the prev patch is not what we're looking for.. bear with me: i'll get it soon! :D

dopry’s picture

Status: Active » Needs work

The patch is close... I'll make you keep fixing it until you get it right. :)

I wouldn't change the if (!is_file($tmpdestination)) block, its a sort of locking mechanism.
This chunk of code prevents multiple apache requests from trying to work on the same file....
Say you get 5 requests for tmpdestination before it is generated and put in its final location....
You have 5 processes trying to manipulate the same file... You won't get the expected results...

While the case is unlikely for any low volume site... any high volumn site after a cache flush might experience bad behavior resulting in horribly mangled images... I'd rather an user see an image not found and things look ok the next page view, that I would like them to see horribly mangled images until I notice them and flush the cache, only to find another image has been horribly mangled...

The else case for that if block should be to deliver a 'placeholder' image for the preset. I haven't implemented that feature yet.

texas-bronius’s picture

Status: Needs work » Active
StatusFileSize
new3.12 KB

Ok. See attached patch. After having created the patch, I changed the filenames in --- and +++ of first two lines to "imagecache.module" manually. Is that correct?..

texas-bronius’s picture

I hate to turn this into a patch thread and mislead other journeymen as I have so fondly been misled in the past myself. My patch above does what it says including the breaking of your is_file check... Can we take this offline (gmail) until I learn how to patch?

dopry’s picture

for generating patches, when I'm not using cvs I normally have a workflow like...

copy example.module to example.module.orig
$ cp example.module example.module.orig

edit example.module
$ vi example.module

generate a diff in unified format from example.module.orig to example.module into a patch file.
$ diff -u example.module.orig example.module > example.patch

hope that helps.

texas-bronius’s picture

StatusFileSize
new592 bytes

The attached patch is against the original 4.7.x-1.x and supercedes all mispatches of mine above. Per dopry's last chastisement (kidding) above, this fixes only Recommendation 1 above, making a unique tmpdestination filename by prepending filename with presentname (not presetid).

thanks for your help d.

texas-bronius’s picture

fyi: when I zap my test files:
rm -fdr files/imagecache
and reload, it works. But my Drupal log still reports
The directory sites/example.com is not writable, because it does not have the correct permissions set

dopry’s picture

Status: Active » Fixed

That last error log you mention is mostly harmless.. It'll just fill your log table after a couple eons.

Commited to the 4.7.-1.x branch.

Anonymous’s picture

Status: Fixed » Closed (fixed)