Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_memoize_version generates invalid keys #131

Open
xealot opened this issue Jan 26, 2016 · 12 comments
Open

_memoize_version generates invalid keys #131

xealot opened this issue Jan 26, 2016 · 12 comments

Comments

@xealot
Copy link

xealot commented Jan 26, 2016

The function _memoize_version which is used within the @cache.memoize() decorator can generate keys that are invalid for memcached.

I am using pylibmc==1.5.0 and Flask-Cache==0.13.1

I instantiate the pylibmc client with the behavior of verify_keys.

cache = Cache(app, config={
    'CACHE_TYPE': 'memcached',
    # The reason we pass in a client directly is because the keys that flask-cache uses somehow break
    # in Python 3. I don't know why, but I managed to trace the code far enough to know that if we were
    # to send a client directly in then the Werkzeug cache backend would use it directly. This, as far
    # as I know is the only way to get Pylibmc options into the client init.
    'CACHE_MEMCACHED_SERVERS': pylibmc.Client(['127.0.0.1:11211'], behaviors={'verify_keys': True}),
})

The reason I do this is because when I switched to Python 3 the cache keys were breaking and verify keys seemed to fix it. As I look back on it now, maybe instead of fixing it it simply caused the errors to be hidden.

I see two potential issues:

  1. You do not look at the response from the set command to see if the set was successful, thus hiding potential issues when setting a key does not succeed.
  2. Some functions use raw un-hashed keys which are incompatible with some cache backends.

Switching my client from pylibmc to python-memcached it properly throws an exception for bad key errors coming from _memoize_version.

@xealot
Copy link
Author

xealot commented Jan 26, 2016

PR #132

@xealot
Copy link
Author

xealot commented Jan 26, 2016

Switching pylibmc to binary=True also seems to avoid this issue. I assume since the key content no longer matters.

@thadeusb
Copy link
Owner

Can you include the exception that you are receiving, as well as a failing unit test?

The pull request proposed causes some of the existing unit tests to fail.

@xealot
Copy link
Author

xealot commented Jan 26, 2016

The exception is:

_pylibmc.SocketCreateError
_pylibmc.SocketCreateError: error 11 from memcached_set_multi: SUCCESS

If you switch to python-memcached you get a more reasonable error. Something along the lines of "Cannot use space/control characters in key".

As far as the PR, I sort of expected it to break stuff. It was mostly to verify that part of the code was the thing generating the fault. I can research adding a failing test.

@thadeusb
Copy link
Owner

Yeah, we need to figure out "why" you are getting invalid keys.

The _memoize_make_cache_key function should be creating an MD5 for you already

https://github.com/thadeusb/flask-cache/blob/master/flask_cache/__init__.py#L402

@xealot
Copy link
Author

xealot commented Jan 26, 2016

That one is easy.

_memoize_make_cache_key calls _memoize_version and that function interacts with the cache directly.

@thadeusb
Copy link
Owner

What is the name of the function you are memoizing?

@thadeusb
Copy link
Owner

I see now it is using the function namespace as-is without any translations

@thadeusb
Copy link
Owner

If you could add a print statement or debug breakpoint to get the value of fetch_keys in the _memoize_version function would be super helpful.

@xealot
Copy link
Author

xealot commented Jan 27, 2016

I've got a unit test for you... Just working on PR.

It will only work if you use memcached as the CACHE_TYPE

@xealot
Copy link
Author

xealot commented Jan 27, 2016

The issue is that some objects have a __repr__ that have spaces for debug/readability and also to make sure that memoize() is unique enough. These spaces cause the cache to fail in the two ways listed above.

@xealot
Copy link
Author

xealot commented Jan 27, 2016

I will retract my earlier comment. Setting the pylibmc binary=True does not fix this issue. The only work around currently is to alter modules to produce spaceless cache keys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants