schmittjoh / JMSSecurityExtraBundle

Enhances the Symfony2 Security Component with several new features
http://jmsyst.com/bundles/JMSSecurityExtraBundle
255 stars 100 forks source link

Verify written cache with graceful fallback #206

Closed jaymecd closed 8 years ago

jaymecd commented 8 years ago

Severity: critical

Within massive traffic and concurrent requests (nginx + php5-fpm), cache file is created with 0 size. Next requests load empty file, what causes ACCESS_DENIED.

This PR checks if exact amount of bytes were written to cache file to avoid being locked by empty file during race condition. Non-blocking I/O is done by writing tmpfile + rename() afterwards. If failed, fallbacks gracefully to eval($source).

Benchmark, why to eval($source) instead of require $file after cache has been written.

Affected versions ^1.2 Tested version: ^1.5 Required to fix versions: at least ^1.5

jaymecd commented 8 years ago

/cc @schmittjoh

schmittjoh commented 8 years ago

Did you consider first writing a temporary file and then renaming that?

jaymecd commented 8 years ago

@schmittjoh nope, but you're right, as LOCK_EX locks file with blocking I/O and all requests have to wait. Will update PR

jaymecd commented 8 years ago

@schmittjoh, PR is updated

schmittjoh commented 8 years ago

Can we verify the written file before renaming instead of each time it is used?

jaymecd commented 8 years ago

updated, smth like this https://github.com/schmittjoh/JMSSecurityExtraBundle/pull/206/files#diff-4794a4cf6e3b7bf956a2df6fbffbc014R154 you've meant?

jaymecd commented 8 years ago

@schmittjoh, build fixed, anything to be corrected?

jaymecd commented 8 years ago

ping @schmittjoh

jaymecd commented 8 years ago

@schmittjoh, would be good to have ur feedback or merge. tnx

jaymecd commented 8 years ago

@schmittjoh, btw, what d'u think on off-load cache file creation into cache warmup process?

jaymecd commented 8 years ago

I've made small updates:

schmittjoh commented 8 years ago

Apart from the comment above, looks good otherwise.

jaymecd commented 8 years ago

@schmittjoh, any updates/ETA regarding this PR? tnx

keradus commented 8 years ago

@schmittjoh please, could we make a new release with this fix ?