Make TryGet reliable #73

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
@vchekan

vchekan commented Jul 22, 2011

I would expect TryGet to never fail and return "false" either when cache miss or exception happen.
But it is not so. No exception is handled by TryGet.

This leads to "poisoned cache" situation. If I save in cache some value which throws an exception when deserializing, it will never be replaced by a good value in this client code (pseudocode):
if(!_cache.TryGet())
_cache.Set(GetValueExpensive())

Instead much more heavier approach needs to be taken with wrapping TryGet into exception and handling it.
Unfortunately "poisoned cache" situation happens not very often and very likely won't be detected in QA cycle. As result it encourage writing a code which is not reliable.

Proposed patch mitigates the issue by wrapping Try* methods implementation into try-catch clause.

@enyim

This comment has been minimized.

Show comment Hide comment
@enyim

enyim Aug 13, 2011

Owner

What kind of exception did you get? Some exceptions MUST be thrown, but it's possible that the exception you encountered was a bug coming from other parts of the code, this doing a generic catch is not a good solution.

Also, could you please reformat your code to match the rest of the files, so i do not have to commit a "cosmetic changes" patch?

(tabs, instead of spaces; follow indentation, etc.)

Owner

enyim commented Aug 13, 2011

What kind of exception did you get? Some exceptions MUST be thrown, but it's possible that the exception you encountered was a bug coming from other parts of the code, this doing a generic catch is not a good solution.

Also, could you please reformat your code to match the rest of the files, so i do not have to commit a "cosmetic changes" patch?

(tabs, instead of spaces; follow indentation, etc.)

@shamilio

This comment has been minimized.

Show comment Hide comment
@shamilio

shamilio Aug 23, 2011

Hello, everybody.

How to distinguish whether the TryGet returns false because of absense of key in the cache or because of the communication failure?

TryGet should throws MemcachedClientException in case of communication failures.

Hello, everybody.

How to distinguish whether the TryGet returns false because of absense of key in the cache or because of the communication failure?

TryGet should throws MemcachedClientException in case of communication failures.

@vchekan

This comment has been minimized.

Show comment Hide comment
@vchekan

vchekan Aug 24, 2011

My particular problem was with a byte-based enum. Such a value successfully serializes but upon deserialization throws an error. As I think about it more, I start realizing that TryGet which never throws may be not the best idea because it would mask situation like mine and but would remain unnoticed for some time.
May be problem lays in associations: I expected it to behave like *.TryParse which would swallow exceptions otherwise thrown by Parse.

vchekan commented Aug 24, 2011

My particular problem was with a byte-based enum. Such a value successfully serializes but upon deserialization throws an error. As I think about it more, I start realizing that TryGet which never throws may be not the best idea because it would mask situation like mine and but would remain unnoticed for some time.
May be problem lays in associations: I expected it to behave like *.TryParse which would swallow exceptions otherwise thrown by Parse.

@enyim

This comment has been minimized.

Show comment Hide comment
@enyim

enyim Aug 24, 2011

Owner

Probably the serialization exceptions should escape from the transcoder which would handle your particular case as well.

I'll give it a try to see how it can be solved because it must be configurable to be backwards compatible with previous versions.

Owner

enyim commented Aug 24, 2011

Probably the serialization exceptions should escape from the transcoder which would handle your particular case as well.

I'll give it a try to see how it can be solved because it must be configurable to be backwards compatible with previous versions.

Enyim recommendations:
Private signature is used only if no other options are provided.
Style fix.
@vchekan

This comment has been minimized.

Show comment Hide comment
@vchekan

vchekan Aug 24, 2011

Fixed signature key according to your recommendations. Thanks for review!

vchekan commented Aug 24, 2011

Fixed signature key according to your recommendations. Thanks for review!

@vchekan

This comment has been minimized.

Show comment Hide comment
@vchekan

vchekan Apr 29, 2013

Will rework those series of patches into smaller ones.

vchekan commented Apr 29, 2013

Will rework those series of patches into smaller ones.

@vchekan vchekan closed this Apr 29, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment