mezzio / mezzio-fastroute

FastRoute integration for Mezzio
https://docs.mezzio.dev/mezzio/features/router/fast-route/
BSD 3-Clause "New" or "Revised" License
16 stars 11 forks source link

Unreliable cache availability check #4

Open sutem opened 3 years ago

sutem commented 3 years ago

https://github.com/mezzio/mezzio-fastroute/blob/9fc90f8921738960a6e61ae951dd5377e59c711f/src/FastRouteRouter.php#L494 The file can be deleted, but its cached in opcache. I think it's better to check through the file_exists.

lcobucci commented 3 years ago

@sutem that is the expected behaviour. You are supposed the clean up cached file and the opcache when deploying a new version.

sutem commented 3 years ago

@lcobucci I completely agree with you. But this is not about deploy. In this situation, we need to be sure that deleting the cache file will trigger the creation of a new one.

lcobucci commented 3 years ago

I don't follow... Is this for development environment? If so, why do have caching enabled?

sutem commented 3 years ago

What I'm trying to say is that include can return a file even if it was deleted, because it remains in the php cache. To prevent this unexpected behavior, you should use file_exists and only then include the file. $dispatchData = file_exists($this->cacheFile) ? include $this->cacheFile : false;

lcobucci commented 3 years ago

That suggestion implies in having extra and unnecessary io calls.

This process is optimised for production environments, if you want to have manual interventions to remove the file you should also take the responsibility for cleaning up the opcache.

Alternatively, you may decide not to use cache which will always give you the updated version of routes.

Does this make sense to you?

sutem commented 3 years ago

That suggestion implies in having extra and unnecessary io calls.

I don't think it's too scary php-bench

This process is optimised for production environments, if you want to have manual interventions to remove the file you should also take the responsibility for cleaning up the opcache.

Clearing opcache is an incorrect option, because the application will depend on the caching technology used.

Alternatively, you may decide not to use cache which will always give you the updated version of routes.

This is also not suitable, because it greatly affects performance.

Does this make sense to you?

I just want others to save time when they encounter my problem, because it is not as simple as it seems. I solved my local problem, but it occurred because the current implementation of the method does ignore php opcode caching.

pine3ree commented 3 years ago

Hello @sutem,

you could also use the opcache_invalidate for the routes cache-file alone, once per request. This should not affect performance (at least it doesn't in ssd disks) and this is what I usually do for files I need to be sure they exists before their opcode expiration (opcache.revalidate_freq).

If we want to add this as an optional feature we must also check that php opcode cache (and not a different kind of opcode caching) is enabled.

kind regards