node-ts-cache icon indicating copy to clipboard operation
node-ts-cache copied to clipboard

Add failsafe option for redis storage

Open adylevy opened this issue 5 years ago • 8 comments

When failSafe passed redis storage won't throw an exception.

adylevy avatar Mar 06 '19 10:03 adylevy

Hi @adylevy

Can you please explain what is the purpose of this feature? Error handling should be managed by the implementing application IMO not by this library.

node-ts-cache uses node_redis client library to integrate redis storage. All options from https://github.com/NodeRedis/node_redis#options-object-properties are supported and there is no such field as failsafe.

havsar avatar Mar 14 '19 12:03 havsar

Hi @havsar , Sorry for the delayed response...

Since the purpose of this lib is to add a decorator above functions for caching purposes an exception in a decorator shouldn't yield an exception within the app but rather a fallthru to the app logic IMO.

For example - when redis is down for example, one would want a response from his app while another will want to return an error msg. since the exception thrown by the decorator will get caught only by the callee having such logic improves the stability of the app.

If there is a way that I'm not familiar with the handle the exception elsewhere while maintaining a proper work of the app LMK. I'd love to learn!

Also, you mentioned redis options, I didn't extend their options but used another param for it.

adylevy avatar Mar 31 '19 13:03 adylevy

@adylevy Thanks for the explaination! You're righ,t an error while connecting to a storage should not break the whole application. I will take a look at your PR and see what's the best way to handle this.

havsar avatar Apr 03 '19 07:04 havsar

@havsar Any news?

adylevy avatar May 07 '19 11:05 adylevy

@havsar ?

adylevy avatar Jul 09 '19 12:07 adylevy

@adylevy Sorry for the super late response! Your failsafeMode in RedisStorage looks good IMO, but I think we can abstract the error handling a little bit more.

What do you think about the following approach for error handling?

const myStrategy = new ExpirationStrategy(new MemoryStorage(), err => log(error))

This way error handling is independent from the storage + the user can decide what to do with the error. Furthermore, the err parameter could be enhanced with data like which method was called, a 'timestamp' and the actual error from the storage like 'RedisError'.

Example:

{ 
  "error":"RedisErrorObject | FsErrorObject | MemoryErrorObject",
  "timestamp":1567892389,
  "cacheKey":"UserService:getUsers:[]"
}

Also what needs to be considered:

  • What happens when an error happens? Should the underlying method be called and the cacheItem deleted? Should the caller be able to set an item manually? Maybe in the error callback with an additional parameter like 'setItem()'?

I will think about a proper solution for error handling.

Suggestions and help is very welcome. @adylevy

havsar avatar Sep 07 '19 21:09 havsar

@adylevy

Will be included in the next release (4.0.0).

havsar avatar Nov 25 '20 14:11 havsar

@adylevy

Will be included in the next release (4.0.0).

Hey @havsar , love the idea of adding err handling to the strategy. waiting for the next release :)

adylevy avatar Apr 25 '21 20:04 adylevy