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

ext_memcached.cpp fails to set compression flags correctly #8028

Open
jhammer opened this issue Oct 17, 2017 · 3 comments · May be fixed by #8031
Open

ext_memcached.cpp fails to set compression flags correctly #8028

jhammer opened this issue Oct 17, 2017 · 3 comments · May be fixed by #8031

Comments

@jhammer
Copy link

jhammer commented Oct 17, 2017

HHVM Version

HipHop VM 3.18.5 (rel)
Compiler: tags/HHVM-3.18.5-0-g61f6a1f9a199c929980408aff866f36a7b4a1515
Repo schema: 514949365dd9d370d84ea5a6db4a3dd3b619e484

Standalone code, or other way to reproduce the problem

When the HHVM implementation of memcached is compressing a value, it correctly sets the MEMC_VAL_COMPRESSED flag, but fails to set the flag that specifies what type of compression is used (either MEMC_VAL_COMPRESSION_FASTLZ or MEMC_VAL_COMPRESSION_ZLIB). This becomes a problem when a non-HHVM implementation of memcached (like php-memcached) tries to read and decompress the value. It can see that the "compressed" flag is set, but doesn't know what compression type it is, and the read fails with a (misleading) error like:

Memcached::get(): could not decompress value: unrecognised encryption type

This HHVM implementation doesn't suffer from this problem because, if it can't determine the compression type, it falls back to decompressing with ZLIB.

I believe the fix is to change line 185 in ext_memcached.cpp from this:

flags |= MEMC_VAL_COMPRESSED;

to this:

flags |= MEMC_VAL_COMPRESSED | MEMC_VAL_COMPRESSION_ZLIB;

@Orvid
Copy link
Contributor

Orvid commented Oct 18, 2017

Could you put up a pull request to make the change?

@atdt
Copy link
Contributor

atdt commented Apr 11, 2018

Ping? The patch still applies cleanly.

@vovagalchenko
Copy link

@Orvid: this seems like a worthwhile patchset for interoperability. Is this something you'd want to merge in the near future?

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

Successfully merging a pull request may close this issue.

4 participants