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

MOD: don't re-set the libmemcached option if not modified #451

Conversation

git-hulk
Copy link
Contributor

@git-hulk git-hulk commented Feb 21, 2020

libmemcached would close all connections if the option(like MEMCACHED_BEHAVIOR_BINARY_PROTOCOL) was set, even though the value wasn't modified. If the user uses the setOptions in the wrong way may cause the persistent connection never works.

@git-hulk
Copy link
Contributor Author

@git-hulk git-hulk commented Feb 25, 2020

@sodabrew can you take time to review this PR?

Copy link
Contributor

@sodabrew sodabrew left a comment

Thank you for debugging this disconnect, I think it may explain several other bugs indicating that Binary mode doesn't perform as expected. Just one style nit noted.

I want to check against any fallout if there are "behaviors" that are set through memcached_behavior_set that are more like Linux ioctls, that can be sort of any value you're trying to pass, and not necessarily set-once settings. I don't know if any such things exist, but I want to confirm before merging.

php_memcached.c Outdated
@@ -3111,6 +3111,11 @@ int php_memc_set_option(php_memc_object_t *intern, long option, zval *value)
lval = zval_get_long(value);

if (flag < MEMCACHED_BEHAVIOR_MAX) {
// do set the option when the option value wasn't modified,
// while libmemcached may shutdown all the connection.
if (memcached_behavior_get(intern->memc,flag) == (uint64_t)lval) {
Copy link
Contributor

@sodabrew sodabrew Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: space after ,

Copy link
Contributor Author

@git-hulk git-hulk Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@git-hulk git-hulk force-pushed the fix/check-value-before-set-option branch from 52a2cbe to b51433b Compare Mar 11, 2020
@git-hulk
Copy link
Contributor Author

@git-hulk git-hulk commented Mar 11, 2020

@sodabrew yeah, it should check the option value was modified or not in libmemcached actually, but it seems that no harm to set-once in php-memcached to avoid the disconnect

@git-hulk git-hulk force-pushed the fix/check-value-before-set-option branch 2 times, most recently from 9a62da1 to 36f18bf Compare Mar 16, 2020
libmemcached would close all connections if the option(like MEMCACHED_BEHAVIOR_BINARY_PROTOCOL)
was set, even though value wasn't modified.
@sodabrew sodabrew merged commit a452d9a into php-memcached-dev:master Oct 9, 2020
1 check passed
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 this pull request may close these issues.

None yet

2 participants