October: `php artisan queue:restart` does not work with RequestCache enabled

[adrenth]

• OctoberCMS Build: 446
• PHP Version: 7.1
• Database Engine: MySQL
• Plugins Installed: none

Cache driver: file

Description:

Restarting a queue (described on the official Laravel 5.5 documentation page https://laravel.com/docs/5.5/queues#running-the-queue-worker)

Steps To Reproduce:

php artisan cache:clear
php artisan config:cache
php artisan queue:work

Open new terminal window:

php artisan queue:restart

Head back to first terminal window. The queue:work process will now gracefully stop.

Start the worker again:

php artisan queue:work

Go to Terminal window 2 and try restart the queue:

php artisan queue:restart

The worker will not stop

Some more context to this issue:

The line $this->laravel['cache']->forever('illuminate:queue:restart', $this->currentTime()); in Illuminate\Queue\Console\RestartCommand will not write the current timestamp to cache.

And therefore will not be detected by the Illuminate/Queue/Worker in the queueShouldRestart method. The timestamps in this method are always the same.

The October\Rain\Halcyon\MemoryRepository is responsible for this I guess.

Simply disabling the request cache will prevent this from happening.

I reported this issue because request caching is enabled by default and will cause issues for users who use the awesomeness of Laravel queues.

config/cache.php

    /*
    |--------------------------------------------------------------------------
    | Disable Request Cache
    |--------------------------------------------------------------------------
    |
    | The request cache stores cache retrievals from the cache store
    | in memory to speed up consecutive retrievals within the same request.
    | Set to true to disable this in-memory request cache.
    |
    */
    'disableRequestCache' => false, // <-- set to TRUE to disable request caching!

[LukeTowers]

@adrenth is there a way that we can disable the cache just for this relevant query or get it to properly detect when it needs to reset that cache item? The request cache is itself an awesome feature of October

[adrenth]

@LukeTowers Don't think you should make an exception for this specific case. I think other users might run into the same issues.

We need to investigate why a cache write such as $this->laravel['cache']->forever('illuminate:queue:restart', $this->currentTime()); does not update the value in the cache.

[LukeTowers]

@adrenth never mind, I thought you were talking about the DB request cache. Are you able to investigate this further and see why this isn't taking effect?

[nathan-van-der-werf]

The worker uses the \October\Rain\Halcyon\MemoryRepository. (vendor/laravel/framework/src/Illuminate/Queue/Console/WorkCommand.php:98)

Since it's a "Memory" cache, it won't ever update the values which are set in a separate process.

When the config option disableRequestCache is true, the worker uses the \Illuminate\Cache\Repository. (vendor/october/rain/src/Halcyon/HalcyonServiceProvider.php:43)

I think the configuration option should be added to config/cache.php anyhow, so users at least know about the existence of it.

[LukeTowers]

@nathan-van-der-werf it is already there: https://github.com/octobercms/october/blob/master/config/cache.php#L95-L106

I think probably the best way to handle this issue is prevent that specific item from going through the request cache

[jimcottrell]

@LukeTowers I agree that's necessary, but I'd argue that we should go a step further for code to work with the memory cache the same in queue workers as in normal page requests. I hadn't actually realized the MemoryRepository was sitting in front of the cache driver by default until I started digging into why a job syncing data to a third party from a queue worker wasn't expiring a cached OAuth token, so my code would never refresh the token the way I was expecting.

Since queue workers can run indefinitely, I would suggest we flush the MemoryRepository on every Illuminate\Queue\Events\JobProcessing event so that each queue job executes with fresh access to the underlying cache, allowing cached items to expire the same as they would if the code were running in another context.

Or maybe there's even an argument to be made that any console command could potentially be long-running enough to cache entries in memory past expected expiration, and MemoryRepository should be automatically disabled for any CLI usage?

Either way, I can put together a PR for this.

[LukeTowers]

@jimcottrell I would say that it's worth disabling the memory cache layer in a CLI context, the original problem that layer was implemented to solve isn't really a big deal in CLI applications so it should be safe to just outright disable it. We could also take it one step further and have the default value be null which would have that behaviour as described but still enable developers to choose to explicitly enable / disable the memory layer. That would probably be the best as it would be inline with other config items present in October.

[jimcottrell]

@LukeTowers Yeah, the more I think about it, the more I think it probably just isn't worth it in a CLI context by default. I like the null/true/false config. 99% of users will probably leave it the default null, and we can add some extra documentation by the config item that if it's explicitly enabled, you should be prepared for the effects on queue workers and long-running console commands.

[LukeTowers]

Go for it @jimcottrell