Concurrent requests for an image that has not yet been cached #38

Closed
kevin-thackorie opened this Issue Feb 26, 2015 · 6 comments

Comments

Projects
None yet
3 participants
@kevin-thackorie

During concurrent requests (even with concurrency as low as 2), if the image has not yet been cached, a FileExistsException gets thrown:

PHP Fatal error: Uncaught exception 'League\Flysystem\FileExistsException' with message 'File already exists at path: 0fb98a01effea237c106d20fc335afb1'

I discovered this race condition issue while benchmarking performance on a VM running locally using ApacheBench v2.3:

ab -n 300 -c 2 http://images.domain.tld/item/test.jpg

@reinink

This comment has been minimized.

Show comment
Hide comment
@reinink

reinink Feb 26, 2015

Member

Interesting. I guess this is possible given that there is a delay between the image being generated and actually being saved to disk.

We definitely need a second check right before saving a new image to disk. Unfortunately, that doesn't help prevent generating an image twice. Probably the simplest solution would be to simply write a blank file immediately, even before the image has been generated. Then, when the manipulated image has been generated, replace the temporary file with it.

Thanks for bringing this to my attention, I'll have to patch this in an upcoming release.

Member

reinink commented Feb 26, 2015

Interesting. I guess this is possible given that there is a delay between the image being generated and actually being saved to disk.

We definitely need a second check right before saving a new image to disk. Unfortunately, that doesn't help prevent generating an image twice. Probably the simplest solution would be to simply write a blank file immediately, even before the image has been generated. Then, when the manipulated image has been generated, replace the temporary file with it.

Thanks for bringing this to my attention, I'll have to patch this in an upcoming release.

@reinink reinink added the bug label Feb 26, 2015

@tmirks

This comment has been minimized.

Show comment
Hide comment
@tmirks

tmirks Feb 26, 2015

@reinink You could have the same issue writing a blank file (i.e. multiple requests trying to write the blank file at the same time). Is there any reason the second request wouldn't just do a silent overwrite (or silent fail)?

There would be a very small performance hit because you're generating it twice, but this would be pretty rare.

tmirks commented Feb 26, 2015

@reinink You could have the same issue writing a blank file (i.e. multiple requests trying to write the blank file at the same time). Is there any reason the second request wouldn't just do a silent overwrite (or silent fail)?

There would be a very small performance hit because you're generating it twice, but this would be pretty rare.

@reinink

This comment has been minimized.

Show comment
Hide comment
@reinink

reinink Feb 26, 2015

Member

Yeah, maybe that's even a better and simpler option, especially given how infrequently this problem will occur. Good thinking Tim!

Member

reinink commented Feb 26, 2015

Yeah, maybe that's even a better and simpler option, especially given how infrequently this problem will occur. Good thinking Tim!

@kevin-thackorie

This comment has been minimized.

Show comment
Hide comment
@kevin-thackorie

kevin-thackorie Feb 26, 2015

+1 for failing silently. Silent overwrite would require changes to Flysystem (the assertAbsent logic in Filesystem.php).

+1 for failing silently. Silent overwrite would require changes to Flysystem (the assertAbsent logic in Filesystem.php).

@reinink

This comment has been minimized.

Show comment
Hide comment
@reinink

reinink Feb 26, 2015

Member

Fixed and tagged!

Member

reinink commented Feb 26, 2015

Fixed and tagged!

@reinink reinink closed this Feb 26, 2015

@tmirks

This comment has been minimized.

Show comment
Hide comment
@tmirks

tmirks Feb 27, 2015

Thanks @reinink, that was quick! 👍

tmirks commented Feb 27, 2015

Thanks @reinink, that was quick! 👍

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