Change session_lock default ini values

[tvlooy]

The old extension had these values as a default. I don't know why this changed, the commit messages don't explain it. People are having issues with it #269 changing the values seems to help.

The max_retries was calculated but defaulted to max_execution_time when it was 0. I chose 30s as the default max_execution_time because this is the default in PHP.

ping @mkoppanen

[sodabrew]

I'd like to do some detective work to understand why the defaults were changed for 3.0 and if it makes sense to change them back. Thank you for taking the discussion in #269 and suggesting specific changes to address the problem reported there.

[tvlooy]

Note that we are running a high traffic (~140 cmd/s) site in production with these settings (we use both sessions and user cache) without any issues so far. We use PHP 7.1.

[jaccoTopVintage]

It would be most interesting to learn the rationale behind changing the default values, as it seems it is the source of quite some problems.

From what I can see, the original values (4bb264e) where:

; Session spin lock retry wait time in microseconds.
; Be carefull when setting this value.
; Valid values are integers, where 0 is interpreted as
; the default value. Negative values result in a reduces
; locking to a try lock.
; the default is 150000
memcached.sess_lock_wait = 150000

; The maximum time, in seconds, to wait for a session lock
; before timing out.
; Setting to 0 results in default behavior, which is to
; use max_execution_time.
memcached.sess_lock_max_wait = 0;

; The time, in seconds, before a lock should release itself.
; Setting to 0 results in the default behaviour, which is to
; use the memcached.sess_lock_max_wait setting. If that is
; also 0, max_execution_time will be used.
memcached.sess_lock_expire = 0;

[sodabrew]

Thanks for taking the time to present a new set of default values that may work better for most people. I will accept this PR, but I merged a cleanup PR ahead of this one that comments-out any default INI values that were explicitly set to the default values. Could you update this PR to reflect the new default values, but retain the commented-out status of the INI directives?

[tvlooy]

@sodabrew I updated the PR

But, ... I also think we can improve the code even more. I'll make a new PR for:

Suggestion 1 (my preference): because nobody knows why the exponential backoff was added, I suggest we remove it and restore the original behavior.

Suggestion 2: change the default value of memcached.sess_lock_retries to -1, which will then take the value of max_execution_time to calculate the actual retries. Which leaves the exponential backoff stuff in for people that want to use it.

My guess is that nobody uses it. So removing it seems the best solution. We can always add it again later when it becomes clear why it was added.

What do you think?

[sodabrew]

Regarding the exponential backoff, that type of algorithm is generally only useful if it is random exponential backoff, so that two requests that begin at exactly the same time will de-conflict after a few tries.

[tvlooy]

@sodabrew so, what's next?

[jaccoTopVintage]

@sodabrew I think the best course of action would be to revert revert back to the old retry algorithm (#355). If reverting to the old retry algorithm takes too much time to make it into the next release, I would suggest reverting back to the 'old' default values (this pull request) as an intermediate quickix.

[tvlooy]

@sodabrew did you find the time to take a look at this and #355 yet? I'm eager to make this extension better

[tvlooy]

Summoning the github php-memcached-dev people @andreiz @dterei @iliaal @krakjoe @mkoppanen @poison @tricky Anyone?

[sodabrew]

Hi @tvlooy I've been waylaid on this project by other work I have going on right now, but I haven't forgotten and will make every effort to merge together the sensible defaults that have been proposed in PR and to cut a release.

[lichunqiang]

Hi all, I had tried this configure for the version 3.0.3, But casued anthor issue, the php slow log tracked session_start function, It's this configuration casued the function slow?

Or it can the server's response? I used a test server, maybe the configuration is too small to handle the frequent request?

libmemcached version 1.0.18

memcached configuration:

memcached support => enabled
memcached.compression_factor => 1.3 => 1.3
memcached.compression_threshold => 2000 => 2000
memcached.compression_type => fastlz => fastlz
memcached.default_binary_protocol => 0 => 0
memcached.default_connect_timeout => 0 => 0
memcached.default_consistent_hash => 0 => 0
memcached.serializer => php => php
memcached.sess_binary_protocol => 1 => 1
memcached.sess_connect_timeout => 0 => 0
memcached.sess_consistent_hash => 1 => 1
memcached.sess_lock_expire => 30 => 30
memcached.sess_lock_max_wait => not set => not set
memcached.sess_lock_retries => 200 => 200
memcached.sess_lock_wait => not set => not set
memcached.sess_lock_wait_max => 150 => 150
memcached.sess_lock_wait_min => 150 => 150
memcached.sess_locking => 1 => 1
memcached.sess_number_of_replicas => 0 => 0
memcached.sess_persistent => 0 => 0
memcached.sess_prefix => memc.sess. => memc.sess.
memcached.sess_randomize_replica_read => 0 => 0
memcached.sess_remove_failed_servers => 0 => 0
memcached.sess_sasl_password => no value => no value
memcached.sess_sasl_username => no value => no value
memcached.sess_server_failure_limit => 0 => 0
memcached.store_retry_count => 2 => 2

update

After full tests for our whole application, there also accoured session_start(): Unable to clear session lock record occasionally

[lukaszklis]

@sodabrew is this PR critical enough to block a release of 3.1.0 (given no updates since November '17 and being carried over for the three milestones already)?

[tvlooy]

I am still running fine with these settings by the way. This issue is open near to a year now. You lost me ... sorry

[sodabrew]

Sold. Will cut a release soon's I can.

[michaelbutler]

Is the comment about sess_lock_wait_min being milliseconds (and not microseconds) correct?

[woolardfa]

@michaelbutler yes.