search

alexedwards / scs

HTTP Session Management for Go
MIT License
2.13k stars 166 forks source link

Error types leak through from storage systems #42

Closed royallthefourth closed 6 years ago

royallthefourth commented 6 years ago

If I invoke a function like GetObject, it sometimes has a non-nil error that comes from the underlying storage system. In order to handle these gracefully, I'd need to program against the underlying storage system to handle something like "cache key not found". I'm specifically talking about the memcached backend here, but it looks like other storage systems use the same leaky abstraction. This defeats the abstraction offered by SCS since I can't easily handle errors generated by multiple backends in the same way. In short, my code that uses scs.Manager shouldn't need to know about memcache.ErrCacheMiss.

To fix this problem, I propose implementing new SCS error types that can map onto the most common storage backend errors in a sensible way:

ErrNotFound
ErrBackendFailure
ErrSomethingElse...

This would provide abstraction over the underlying errors without hiding too much about what's really happening. I'd be happy to program this myself and rework all of the storage backends to use it.

What do you think?

alexedwards commented 6 years ago

The leaking of memcache.ErrCacheMiss was a bug with the recently contributed memcached store. The Find method should have been returning nil in the event of a cache miss (not an error) as per the store interface documentation.

It's my fault for taking the tests at face-value, instead of checking the tests to make sure they were testing the right thing.

I've fixed the behaviour in commit https://github.com/alexedwards/scs/commit/a01923edcb7f83c0c6ae7e70ad03f293e6ec9e26 so it now works as documented. In the event of a cache miss, Find() will return nil, false, nil instead of nil, false, memcache.ErrCacheMiss and the session manager will create a new session as appropriate.

As for abstracting the other error types, I'm not sure if it's necessary. Generally these will be things that you would want to log and send the user a 500 Internal Server Error response for. For debugging purposes, my gut feeling is that it's better to pass through the precise, specific error so it can be logged. Having an abstraction layer, so that only scs.ErrBackendFailure (or similar) gets logged, would make identifying and debugging the underlying problem more difficult.

royallthefourth commented 6 years ago

Oh, sorry, I missed that section of the docs. Didn't mean to dump the responsibility of fixing my storage backend onto you.