monospice / laravel-redis-sentinel-drivers

Redis Sentinel integration for Laravel and Lumen.
MIT License
101 stars 48 forks source link

Driver [redis-sentinel] is not supported. #17

Closed adamzwakk closed 4 years ago

adamzwakk commented 5 years ago

Hey guys,

Just installing the driver and setting the .env files, and I'm getting the error Driver [redis-sentinel] is not supported. on my Laraval 5.5 instance (it seems to hit that in Illuminate/Cache/CacheManager.php)

I'm using the default vars

CACHE_DRIVER=redis-sentinel
SESSION_DRIVER=redis-sentinel
QUEUE_DRIVER=redis-sentinel
REDIS_DRIVER=redis-sentinel

REDIS_HOST=localhost
REDIS_PORT=26379
REDIS_SENTINEL_SERVICE=mymaster
REDIS_CACHE_DATABASE=1
REDIS_SESSION_DATABASE=2
REDIS_QUEUE_DATABASE=3

with localhost being where redis/redis-sentinel is installed, but it doesn't seem to see the driver. Any ideas? Did I miss something?

cyrossignol commented 5 years ago

Hey Adam, the package's service provider needs to boot() before the drivers become available. Laravel does this automatically, but the cache service cannot find the Sentinel driver if something in the application tries to access the cache before that happens (for example, if another service provider uses the cache in its register() method).

If you're using Laravel 5.5's package autoloading to register this package's service provider, and your project uses another package with a service provider that happens to autoload before this one (and it uses the cache), the driver may not be available yet because Laravel executes the other one first. You can manually register this package's service provider in config/app.php to fix the dependency order.

adamzwakk commented 5 years ago

I've added it to "dont-discover" list in composer.json, and trying to load it manually at the top of app.php, same result.

What's interesting though if I don't have the provider entry, the error turns into Cache store [redis-sentinel] is not defined. which is expected, but with the entry, I get Driver [redis-sentinel] is not supported.

So it's definitely being detected in some way anyways, I'll keep poking at it

cyrossignol commented 5 years ago

As you found, putting it at the top of the 'providers' array in config/app.php before Laravel's built-in service providers won't work because the binding gets wiped out when Laravel's own cache service provider executes afterward.

Try putting the entry after Laravel's built-in providers, but before any other package or app-specific providers. The scenarios you described above seem to validate the theory that something's trying to access the cache service before the package's provider boots. If you get the same error after this, try to find a service provider that uses the cache in its register() method. If you find one, you may need to move that logic to the provider's boot() method. The stack trace should help to identify where the cache is being accessed from.

adamzwakk commented 5 years ago

The offending package seems to be Waavi/translation, commenting out that package in favor the default one seems to let sentinel load :)

I'll poke them and ref this issue, thanks for the help!

dave-lang commented 5 years ago

I'm also running into this issue with Lumen 5.7. The update defers the provider boot until passed to the Kernel/router to handle, that breaks the Kernel as it references Cache prior to booting the providers as part of setting up the schedule (https://github.com/laravel/lumen-framework/blob/26e830b9e81eb4dff2b1df0f5b0755cb7a5e9cbe/src/Console/Kernel.php#L97-L99).

Previous versions booted providers immediately so there wasn't any issue. I'm working around it by booting the providers before handing off to the Kernel but given how vital cache is it might be worth setting up immediately instead of deferring?

cyrossignol commented 5 years ago

Dave, thanks for bringing that up. I'd consider that a bug in Lumen--the framework isn't respecting the convention that Laravel sets for the interoperability of its service providers. At this point, however, it's probably easier to to just update the package.

Personally, I don't mind booting the drivers during provider registration. I considered writing the package that way initially, but I followed Laravel's convention instead. This is a breaking change, though. I think we can add a config directive at first (redis-sentinel.auto_boot = true) and refactor the package later if we can get rid of the overhead.

Let me know your thoughts.

dave-lang commented 5 years ago

Thanks for the quick reply, I agree - it's a Lumen issue and I've logged it on the framework (https://github.com/laravel/lumen-framework/issues/844).

I think an auto boot config for would be a good addition, not just for framework issues but for handling inter-op issues with other providers and packages that don't follow the recommended register/boot convention.

It should be a simple, if autoboot set (config or env) boot the provider, if booting check whether already booted (as the framework will try to re-boot it). I can submit a pull request tonight if needed?

cyrossignol commented 5 years ago

That sounds great--thanks! I'm sure @adamzwakk will be happy for the fix as well.

matrad commented 5 years ago

The Same issue ocurrs when overriding the SessionServiceProvider, like described here.

I've found no way to solve the Driver [redis-sentinel] not supported. issue.

cyrossignol commented 5 years ago

@matrad Sorry, I missed the email for your comment.

This sounds like the same issue described in the rest of this ticket: one of the service providers in your application seems like it's trying to call Laravel's session service during the registration phase before this package's service provider boots.

I merged @davoaust's changes that provide a workaround this issue. Please install the 2.x-dev version of this package and set REDIS_SENTINEL_AUTO_BOOT=true in the .env file.

If this works for you, I will release a new stable version of this package with these changes.

cyrossignol commented 4 years ago

Auto-boot improvements by @dave-lang finally released in version 2.6.0. This provides a workaround for applications and other packages that depend on Sentinel services before the application finishes the boot() phase.

Please comment if the issue persists after enabling the package's auto-boot option.

ahmadalli commented 4 years ago

I'm trying to setup https://github.com/laravel/telescope and I'm facing the same issue. I'm still getting Driver [redis-sentinel] is not supported. error even after setting REDIS_SENTINEL_AUTO_BOOT=true env.

ahmadalli commented 4 years ago

I've solved my issue by installing telescope as dev dependency

cyrossignol commented 4 years ago

Thanks for the report @ahmadalli. I'm glad you resolved the issue.

For future readers: this package's auto-boot workaround will not work in some cases when using Laravel's package auto-discovery feature because Laravel may load this package's service provider after the other problematic package.

Unfortunately, this means we need to declare the service provider order manually in some applications by registering them in config/app.php. The auto-boot option fixes compatibility with packages that try to use Sentinel services during the provider registration phase, but it cannot override the service provider registration order generated by the framework's package auto-discovery.