search

mage2tv / magento-cache-clean

A faster drop in replacement for bin/magento cache:clean with file watcher
BSD 3-Clause "New" or "Revised" License
528 stars 61 forks source link

Uncaught Exception if redis is offline #24

Closed riconeitzel closed 5 years ago

riconeitzel commented 5 years ago

My redis wasn't up and running but cc.js tried to clean parts in redis which resulted in the following error message:

Watcher initialized (Ctrl-C to quit)
Cleaning cache type(s) config
events.js:183
      throw er; // Unhandled 'error' event
      ^

Error: Redis connection to 127.0.0.1:6379 failed - connect ECONNREFUSED 127.0.0.1:6379
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1191:14)
Vinai commented 5 years ago

Thanks, I'll see if I can turn that into a nice message.

Vinai commented 5 years ago

Thinking about how to make that better, nothing really comes to mind. The message is pretty clear, and there is no clear way for the application to recover. At the moment I'm tempted to leave that behavior as it is. Any suggestions?

riconeitzel commented 5 years ago

How about just rendering a message and continue to work. I just restarted redis but then I had to restart the watcher (that’s truly a convenience function - feel free to close the ticket)

The word „uncaught“ triggered me to submit an issue report.

Vinai commented 5 years ago

Understood. After taking a look at this, it's slightly more complicated than it seemed at first because fo two reasons. First, wrapping everything in a try/catch isn't straight forward because of the async nature of node_redis. Catching the error needs to be done through a custom retry_handler, and because the retry handler callback is also async, it can't influence the usual flow of the program easily. I would have to change a lot of structure to enable this. Secondly, I'm only reading the Magento configuration once during the initialization of the program. This is done by shelling out to PHP to parse and spit out the app/etc/env.php file as JSON. The value is then kept in memory while it is running. This is done to make flushing the cache as quick possible. The connection to redis is made only when some cache tags or cache IDs need to be flushed. Changing this also would require a fair bit of restructuring.

Because of the amount of work involved and the marginal benefit I see I'll close this as a wont-fix.