Use tempnam() to give each imagecache action a unique file name

Seems the $preset path is removed in this patch...

Nevermind, that is the temp destination. No longer needed because of tempnam()

Thanks Mark! Committed! Those orphaned tmp files are a killer!

I changed str_replace(dirname($path) . '/', '', $path) with just basename($path) as it seemed a simpler way of accomplishing the same goal.

Thanks Mark! Committed! Those orphaned tmp files are a killer!

I changed str_replace(dirname($path) . '/', '', $path) with just basename($path) as it seemed a simpler way of accomplishing the same goal.

The semi unique filename was in place for a reason, To prevent multiple requests for the same cache image at the same time. The example would be a heavily loaded server whose imagecache was recently cleared. Is that no longer an issue? Will race conditions occur as a result of this change? Is there a reason dangling tmp files are being left behind?

I'm going to use an exaggeration to illustrate my last point.... if imagecache were modified to handle audio transcoding, a process that could take several minutes, you open your server to beginning a transcode process for every request for files/imagecache/128kmp3/files/somefile.flac until files/imagecache/128kmp3/files/somefile.flac is actually created....

Dopry, I agree with what you've said but this seemed like the lesser of two evils. During a race condition, there is still a check in place to see if the destination file exists, if it does then the processing is aborted and the destination file is returned.

If the destination file doesn't exist then true... now php is creating multiple copies of the same image. However, the image always gets served back to the client in a complete form, rather than what happened before when PHP would check for a temp file, if it existed (even in an incomplete form) it would get served back the client as a broken image.

Thinking about it though, that means that the final file_move() should probably be FILE_EXISTS_REPLACE, rather than _RENAME (the default). I'l look into this some more.

Dangling temp files could occur during any Apache (hard) restart or crash. The end result would be those files might not be created for days or weeks until the server is restarted and /tmp is cleared out.

The the real issue is 1) what do we serve when an file is being processed and 2) how do we deal with garbage collection in drupal's tmp directory.

1) we can serve a placeholder image instead. Its a pending feature request, no?
2) we need to add a cron call that cleans up files over 4 hours old in drupal's tmp directory, and should probably be included in system_cron. This was discussed in the 4.7 release cycle, but abandoned as being a fringe case. I never got back around to it in the 5.0 cycle.

Hmm, that's a better point. My assumption was built on the processing taking only a short period of time.

On a practical test (I just completed). We have a website that gets a substantial amount of traffic. Images are requested about 5 per second, and we have about 6000 images. I cleared the cache for a thumbnail preset and viewing the directory, I didn't get any duplicate images (a sign that two processings occurred on one image).

Either way. I see your point. Perhaps using the previous method, but checking the modified time on the temp file would be a better approach?

I rolled back these changes to opt for a system that does a little garbage collection. Checking the file modified time, we can accomplish the same goal in a race situation and not have dead files cause us troubles. I'll get to this tomorrow I think, but Mark or Darrel if you want to beat me to it go ahead. :)

Hi, is there any news on this issue?

thanks

I've been thinking shutdown or exit would be a good spot to clean up those dangling lock files.

I added a shutdown function to remove the lock file... I'll probably release a 1.4 in the next week or two...