Skip to content

Commit 687a0b4

Browse files
mgriegomichael-grunder
mgriego
authored and
michael-grunder
committed
Fail if session lock can't be acquired, more sane lock wait defaults, and add more logging.
1 parent 05cb508 commit 687a0b4

File tree

4 files changed

+45
-36
lines changed

4 files changed

+45
-36
lines changed

‎.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,3 +17,4 @@ run-tests.php
1717
idea/*
1818
.cquery
1919
tags
20+
.vscode/*

‎README.markdown‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,10 +91,10 @@ Following INI variables can be used to configure session locking:
9191
redis.session.locking_enabled = 1
9292
; How long should the lock live (in seconds)? Defaults to: value of max_execution_time.
9393
redis.session.lock_expire = 60
94-
; How long to wait between attempts to acquire lock, in microseconds (µs)?. Defaults to: 2000
94+
; How long to wait between attempts to acquire lock, in microseconds (µs)?. Defaults to: 20000
9595
redis.session.lock_wait_time = 50000
96-
; Maximum number of times to retry (-1 means infinite). Defaults to: 10
97-
redis.session.lock_retries = 10
96+
; Maximum number of times to retry (-1 means infinite). Defaults to: 1000
97+
redis.session.lock_retries = 2000
9898
~~~
9999

100100
## Distributed Redis Array

‎redis.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,8 @@ PHP_INI_BEGIN()
103103
/* redis session */
104104
PHP_INI_ENTRY("redis.session.locking_enabled", "0", PHP_INI_ALL, NULL)
105105
PHP_INI_ENTRY("redis.session.lock_expire", "0", PHP_INI_ALL, NULL)
106-
PHP_INI_ENTRY("redis.session.lock_retries", "10", PHP_INI_ALL, NULL)
107-
PHP_INI_ENTRY("redis.session.lock_wait_time", "2000", PHP_INI_ALL, NULL)
106+
PHP_INI_ENTRY("redis.session.lock_retries", "1000", PHP_INI_ALL, NULL)
107+
PHP_INI_ENTRY("redis.session.lock_wait_time", "20000", PHP_INI_ALL, NULL)
108108
PHP_INI_END()
109109

110110
/** {{{ Argument info for commands in redis 1.0 */

‎redis_session.c‎

Lines changed: 39 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -231,13 +231,13 @@ static int lock_acquire(RedisSock *redis_sock, redis_session_lock_status *lock_s
231231
/* How long to wait between attempts to acquire lock */
232232
lock_wait_time = INI_INT("redis.session.lock_wait_time");
233233
if (lock_wait_time == 0) {
234-
lock_wait_time = 2000;
234+
lock_wait_time = 20000;
235235
}
236236

237237
/* Maximum number of times to retry (-1 means infinite) */
238238
retries = INI_INT("redis.session.lock_retries");
239239
if (retries == 0) {
240-
retries = 10;
240+
retries = 1000;
241241
}
242242

243243
/* How long should the lock live (in seconds) */
@@ -323,7 +323,7 @@ static int write_allowed(RedisSock *redis_sock, redis_session_lock_status *lock_
323323
* if we aren't flagged as locked, so if we're not flagged here something
324324
* failed */
325325
if (!lock_status->is_locked) {
326-
php_error_docref(NULL, E_WARNING, "Failed to refresh session lock");
326+
php_error_docref(NULL, E_WARNING, "Session lock expired");
327327
}
328328
}
329329

@@ -454,13 +454,19 @@ PS_OPEN_FUNC(redis)
454454
}
455455

456456
if ((url->path == NULL && url->host == NULL) || weight <= 0 || timeout <= 0) {
457+
char *path = estrndup(save_path+i, j-i);
458+
php_error_docref(NULL, E_WARNING,
459+
"Failed to parse session.save_path (error at offset %d, url was '%s')", i, path);
460+
efree(path);
461+
457462
php_url_free(url);
458463
if (persistent_id) zend_string_release(persistent_id);
459464
if (prefix) zend_string_release(prefix);
460465
if (user) zend_string_release(user);
461466
if (pass) zend_string_release(pass);
462467
redis_pool_free(pool);
463468
PS_SET_MOD_DATA(NULL);
469+
464470
return FAILURE;
465471
}
466472

@@ -568,9 +574,7 @@ PS_CREATE_SID_FUNC(redis)
568574
RedisSock *redis_sock = rpm ? rpm->redis_sock : NULL;
569575

570576
if (!redis_sock) {
571-
php_error_docref(NULL, E_NOTICE,
572-
"Redis not available while creating session_id");
573-
577+
php_error_docref(NULL, E_NOTICE, "Redis connection not available");
574578
zend_string_release(sid);
575579
return php_session_create_id(NULL);
576580
}
@@ -588,7 +592,7 @@ PS_CREATE_SID_FUNC(redis)
588592
sid = NULL;
589593
}
590594

591-
php_error_docref(NULL, E_NOTICE,
595+
php_error_docref(NULL, E_WARNING,
592596
"Acquiring session lock failed while creating session_id");
593597

594598
return NULL;
@@ -611,23 +615,21 @@ PS_VALIDATE_SID_FUNC(redis)
611615
redis_pool_member *rpm = redis_pool_get_sock(pool, skey);
612616
RedisSock *redis_sock = rpm ? rpm->redis_sock : NULL;
613617
if (!redis_sock) {
618+
php_error_docref(NULL, E_WARNING, "Redis connection not available");
614619
return FAILURE;
615620
}
616621

617622
/* send EXISTS command */
618623
zend_string *session = redis_session_key(redis_sock, skey, skeylen);
619624
cmd_len = REDIS_SPPRINTF(&cmd, "EXISTS", "S", session);
620625
zend_string_release(session);
621-
if (redis_sock_write(redis_sock, cmd, cmd_len) < 0) {
626+
if (redis_sock_write(redis_sock, cmd, cmd_len) < 0 || (response = redis_sock_read(redis_sock, &response_len)) == NULL) {
627+
php_error_docref(NULL, E_WARNING, "Error communicating with Redis server");
622628
efree(cmd);
623629
return FAILURE;
624630
}
625-
efree(cmd);
626631

627-
/* read response */
628-
if ((response = redis_sock_read(redis_sock, &response_len)) == NULL) {
629-
return FAILURE;
630-
}
632+
efree(cmd);
631633

632634
if (response_len == 2 && response[0] == ':' && response[1] == '1') {
633635
efree(response);
@@ -655,6 +657,7 @@ PS_UPDATE_TIMESTAMP_FUNC(redis)
655657
redis_pool_member *rpm = redis_pool_get_sock(pool, skey);
656658
RedisSock *redis_sock = rpm ? rpm->redis_sock : NULL;
657659
if (!redis_sock) {
660+
php_error_docref(NULL, E_WARNING, "Redis connection not available");
658661
return FAILURE;
659662
}
660663

@@ -663,16 +666,13 @@ PS_UPDATE_TIMESTAMP_FUNC(redis)
663666
cmd_len = REDIS_SPPRINTF(&cmd, "EXPIRE", "Sd", session, session_gc_maxlifetime());
664667
zend_string_release(session);
665668

666-
if (redis_sock_write(redis_sock, cmd, cmd_len) < 0) {
669+
if (redis_sock_write(redis_sock, cmd, cmd_len) < 0 || (response = redis_sock_read(redis_sock, &response_len)) == NULL) {
670+
php_error_docref(NULL, E_WARNING, "Error communicating with Redis server");
667671
efree(cmd);
668672
return FAILURE;
669673
}
670-
efree(cmd);
671674

672-
/* read response */
673-
if ((response = redis_sock_read(redis_sock, &response_len)) == NULL) {
674-
return FAILURE;
675-
}
675+
efree(cmd);
676676

677677
if (response_len == 2 && response[0] == ':') {
678678
efree(response);
@@ -699,6 +699,7 @@ PS_READ_FUNC(redis)
699699
redis_pool_member *rpm = redis_pool_get_sock(pool, skey);
700700
RedisSock *redis_sock = rpm ? rpm->redis_sock : NULL;
701701
if (!redis_sock) {
702+
php_error_docref(NULL, E_WARNING, "Redis connection not available");
702703
return FAILURE;
703704
}
704705

@@ -708,20 +709,24 @@ PS_READ_FUNC(redis)
708709
cmd_len = REDIS_SPPRINTF(&cmd, "GET", "S", pool->lock_status.session_key);
709710

710711
if (lock_acquire(redis_sock, &pool->lock_status) != SUCCESS) {
711-
php_error_docref(NULL, E_NOTICE,
712-
"Acquire of session lock was not successful");
712+
php_error_docref(NULL, E_WARNING, "Failed to acquire session lock");
713+
efree(cmd);
714+
return FAILURE;
713715
}
714716

715717
if (redis_sock_write(redis_sock, cmd, cmd_len) < 0) {
718+
php_error_docref(NULL, E_WARNING, "Error communicating with Redis server");
716719
efree(cmd);
717720
return FAILURE;
718721
}
722+
719723
efree(cmd);
720724

721725
/* Read response from Redis. If we get a NULL response from redis_sock_read
722726
* this can indicate an error, OR a "NULL bulk" reply (empty session data)
723727
* in which case we can reply with success. */
724728
if ((resp = redis_sock_read(redis_sock, &resp_len)) == NULL && resp_len != -1) {
729+
php_error_docref(NULL, E_WARNING, "Error communicating with Redis server");
725730
return FAILURE;
726731
}
727732

@@ -751,6 +756,7 @@ PS_WRITE_FUNC(redis)
751756
redis_pool_member *rpm = redis_pool_get_sock(pool, skey);
752757
RedisSock *redis_sock = rpm ? rpm->redis_sock : NULL;
753758
if (!redis_sock) {
759+
php_error_docref(NULL, E_WARNING, "Redis connection not available");
754760
return FAILURE;
755761
}
756762

@@ -760,21 +766,25 @@ PS_WRITE_FUNC(redis)
760766
cmd_len = REDIS_SPPRINTF(&cmd, "SETEX", "Sds", session, session_gc_maxlifetime(), sval, svallen);
761767
zend_string_release(session);
762768

763-
if (!write_allowed(redis_sock, &pool->lock_status) || redis_sock_write(redis_sock, cmd, cmd_len ) < 0) {
769+
if (!write_allowed(redis_sock, &pool->lock_status)) {
770+
php_error_docref(NULL, E_WARNING, "Unable to write session: session lock not held");
764771
efree(cmd);
765772
return FAILURE;
766773
}
767-
efree(cmd);
768774

769-
/* read response */
770-
if ((response = redis_sock_read(redis_sock, &response_len)) == NULL) {
775+
if (redis_sock_write(redis_sock, cmd, cmd_len ) < 0 || (response = redis_sock_read(redis_sock, &response_len)) == NULL) {
776+
php_error_docref(NULL, E_WARNING, "Error communicating with Redis server");
777+
efree(cmd);
771778
return FAILURE;
772779
}
773780

781+
efree(cmd);
782+
774783
if (IS_REDIS_OK(response, response_len)) {
775784
efree(response);
776785
return SUCCESS;
777786
} else {
787+
php_error_docref(NULL, E_WARNING, "Error writing session data to Redis: %s", response);
778788
efree(response);
779789
return FAILURE;
780790
}
@@ -794,6 +804,7 @@ PS_DESTROY_FUNC(redis)
794804
redis_pool_member *rpm = redis_pool_get_sock(pool, skey);
795805
RedisSock *redis_sock = rpm ? rpm->redis_sock : NULL;
796806
if (!redis_sock) {
807+
php_error_docref(NULL, E_WARNING, "Redis connection not available");
797808
return FAILURE;
798809
}
799810

@@ -804,16 +815,13 @@ PS_DESTROY_FUNC(redis)
804815
zend_string *session = redis_session_key(redis_sock, skey, skeylen);
805816
cmd_len = REDIS_SPPRINTF(&cmd, "DEL", "S", session);
806817
zend_string_release(session);
807-
if (redis_sock_write(redis_sock, cmd, cmd_len) < 0) {
818+
if (redis_sock_write(redis_sock, cmd, cmd_len) < 0 || (response = redis_sock_read(redis_sock, &response_len)) == NULL) {
819+
php_error_docref(NULL, E_WARNING, "Error communicating with Redis server");
808820
efree(cmd);
809821
return FAILURE;
810822
}
811-
efree(cmd);
812823

813-
/* read response */
814-
if ((response = redis_sock_read(redis_sock, &response_len)) == NULL) {
815-
return FAILURE;
816-
}
824+
efree(cmd);
817825

818826
if (response_len == 2 && response[0] == ':' && (response[1] == '0' || response[1] == '1')) {
819827
efree(response);

0 commit comments

Comments
 (0)