Skip to content

PHPC-1151: Maintain Session reference on Cursor #801

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 16, 2018
Merged

PHPC-1151: Maintain Session reference on Cursor #801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
06f6080
Move up client argument for phongo_parse_session()
jmikola Apr 12, 2018
21307de
PHPC-1151: Maintain Session reference on Cursor
jmikola Apr 12, 2018
cc90304
PHPC-1151: Add Session to Cursor debug output
jmikola Apr 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 27 additions & 13 deletions php_phongo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ static void php_phongo_log(mongoc_log_level_t log_level, const char *log_domain,
/* }}} */

/* {{{ Init objects */
static void phongo_cursor_init(zval *return_value, mongoc_client_t *client, mongoc_cursor_t *cursor, zval *readPreference TSRMLS_DC) /* {{{ */
static void phongo_cursor_init(zval *return_value, mongoc_client_t *client, mongoc_cursor_t *cursor, zval *readPreference, zval *session TSRMLS_DC) /* {{{ */
{
php_phongo_cursor_t *intern;

Expand All @@ -256,15 +256,24 @@ static void phongo_cursor_init(zval *return_value, mongoc_client_t *client, mong
#else
Z_ADDREF_P(readPreference);
intern->read_preference = readPreference;
#endif
}

if (session) {
#if PHP_VERSION_ID >= 70000
ZVAL_ZVAL(&intern->session, session, 1, 0);
#else
Z_ADDREF_P(session);
intern->session = session;
#endif
}
} /* }}} */

static void phongo_cursor_init_for_command(zval *return_value, mongoc_client_t *client, mongoc_cursor_t *cursor, const char *db, zval *command, zval *readPreference TSRMLS_DC) /* {{{ */
static void phongo_cursor_init_for_command(zval *return_value, mongoc_client_t *client, mongoc_cursor_t *cursor, const char *db, zval *command, zval *readPreference, zval *session TSRMLS_DC) /* {{{ */
{
php_phongo_cursor_t *intern;

phongo_cursor_init(return_value, client, cursor, readPreference TSRMLS_CC);
phongo_cursor_init(return_value, client, cursor, readPreference, session TSRMLS_CC);
intern = Z_CURSOR_OBJ_P(return_value);

intern->database = estrdup(db);
Expand All @@ -277,11 +286,11 @@ static void phongo_cursor_init_for_command(zval *return_value, mongoc_client_t *
#endif
} /* }}} */

static void phongo_cursor_init_for_query(zval *return_value, mongoc_client_t *client, mongoc_cursor_t *cursor, const char *namespace, zval *query, zval *readPreference TSRMLS_DC) /* {{{ */
static void phongo_cursor_init_for_query(zval *return_value, mongoc_client_t *client, mongoc_cursor_t *cursor, const char *namespace, zval *query, zval *readPreference, zval *session TSRMLS_DC) /* {{{ */
{
php_phongo_cursor_t *intern;

phongo_cursor_init(return_value, client, cursor, readPreference TSRMLS_CC);
phongo_cursor_init(return_value, client, cursor, readPreference, session TSRMLS_CC);
intern = Z_CURSOR_OBJ_P(return_value);

/* namespace has already been validated by phongo_execute_query() */
Expand Down Expand Up @@ -540,11 +549,13 @@ bool phongo_parse_read_preference(zval *options, zval **zreadPreference TSRMLS_D
return true;
} /* }}} */

/* Parses the "session" option for an execute method. If mongoc_opts is not
* NULL, the option will be appended. If zsession is not NULL, it will be
/* Parses the "session" option for an execute method. The client object should
* correspond to the Manager executing the operation and will be used to ensure
* that the session is correctly associated with that client. If mongoc_opts is
* not NULL, the option will be appended. If zsession is not NULL, it will be
* assigned to the option. On error, false is returned and an exception is
* thrown. */
static bool phongo_parse_session(zval *options, bson_t *mongoc_opts, zval **zsession, mongoc_client_t *client TSRMLS_DC) /* {{{ */
static bool phongo_parse_session(zval *options, mongoc_client_t *client, bson_t *mongoc_opts, zval **zsession TSRMLS_DC) /* {{{ */
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for moving the argument order around?

Copy link
Member Author

Choose a reason for hiding this comment

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

See commit message: 0b6f04f

{
zval *option = NULL;
const mongoc_client_session_t *client_session;
Expand Down Expand Up @@ -652,7 +663,7 @@ bool phongo_execute_bulk_write(mongoc_client_t *client, const char *namespace, p
return false;
}

if (!phongo_parse_session(options, NULL, &zsession, client TSRMLS_CC)) {
if (!phongo_parse_session(options, client, NULL, &zsession TSRMLS_CC)) {
/* Exception should already have been thrown */
return false;
}
Expand Down Expand Up @@ -746,6 +757,7 @@ int phongo_execute_query(mongoc_client_t *client, const char *namespace, zval *z
char *collname;
mongoc_collection_t *collection;
zval *zreadPreference = NULL;
zval *zsession = NULL;

if (!phongo_split_namespace(namespace, &dbname, &collname)) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "%s: %s", "Invalid namespace provided", namespace);
Expand All @@ -767,7 +779,7 @@ int phongo_execute_query(mongoc_client_t *client, const char *namespace, zval *z
return false;
}

if (!phongo_parse_session(options, query->opts, NULL, client TSRMLS_CC)) {
if (!phongo_parse_session(options, client, query->opts, &zsession TSRMLS_CC)) {
/* Exception should already have been thrown */
mongoc_collection_destroy(collection);
return false;
Expand Down Expand Up @@ -797,7 +809,8 @@ int phongo_execute_query(mongoc_client_t *client, const char *namespace, zval *z
return true;
}

phongo_cursor_init_for_query(return_value, client, cursor, namespace, zquery, zreadPreference TSRMLS_CC);
phongo_cursor_init_for_query(return_value, client, cursor, namespace, zquery, zreadPreference, zsession TSRMLS_CC);

return true;
} /* }}} */

Expand All @@ -823,6 +836,7 @@ int phongo_execute_command(mongoc_client_t *client, php_phongo_command_type_t ty
bson_t opts = BSON_INITIALIZER;
mongoc_cursor_t *cmd_cursor;
zval *zreadPreference = NULL;
zval *zsession = NULL;
int result;

command = Z_COMMAND_OBJ_P(zcommand);
Expand All @@ -839,7 +853,7 @@ int phongo_execute_command(mongoc_client_t *client, php_phongo_command_type_t ty
return false;
}

if (!phongo_parse_session(options, &opts, NULL, client TSRMLS_CC)) {
if (!phongo_parse_session(options, client, &opts, &zsession TSRMLS_CC)) {
/* Exception should already have been thrown */
bson_destroy(&opts);
return false;
Expand Down Expand Up @@ -920,7 +934,7 @@ int phongo_execute_command(mongoc_client_t *client, php_phongo_command_type_t ty
bson_destroy(&reply);
}

phongo_cursor_init_for_command(return_value, client, cmd_cursor, db, zcommand, zreadPreference TSRMLS_CC);
phongo_cursor_init_for_command(return_value, client, cmd_cursor, db, zcommand, zreadPreference, zsession TSRMLS_CC);
return true;
} /* }}} */
/* }}} */
Expand Down
1 change: 1 addition & 0 deletions php_phongo_structs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ typedef struct {
PHONGO_STRUCT_ZVAL query;
PHONGO_STRUCT_ZVAL command;
PHONGO_STRUCT_ZVAL read_preference;
PHONGO_STRUCT_ZVAL session;
PHONGO_ZEND_OBJECT_POST
} php_phongo_cursor_t;

Expand Down
16 changes: 16 additions & 0 deletions src/MongoDB/Cursor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,10 @@ static void php_phongo_cursor_free_object(phongo_free_object_arg *object TSRMLS_
zval_ptr_dtor(&intern->read_preference);
}

if (!Z_ISUNDEF(intern->session)) {
zval_ptr_dtor(&intern->session);
}

php_phongo_cursor_free_current(intern);

#if PHP_VERSION_ID < 70000
Expand Down Expand Up @@ -486,6 +490,18 @@ static HashTable *php_phongo_cursor_get_debug_info(zval *object, int *is_temp TS
ADD_ASSOC_NULL_EX(&retval, "readPreference");
}

if (!Z_ISUNDEF(intern->session)) {
#if PHP_VERSION_ID >= 70000
ADD_ASSOC_ZVAL_EX(&retval, "session", &intern->session);
Z_ADDREF(intern->session);
#else
ADD_ASSOC_ZVAL_EX(&retval, "session", intern->session);
Z_ADDREF_P(intern->session);
#endif
} else {
ADD_ASSOC_NULL_EX(&retval, "session");
}

ADD_ASSOC_BOOL_EX(&retval, "isDead", !mongoc_cursor_is_alive(intern->cursor));

ADD_ASSOC_LONG_EX(&retval, "currentIndex", intern->current);
Expand Down
38 changes: 38 additions & 0 deletions tests/cursor/bug1151-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
--TEST--
PHPC-1151: Segfault if session unset before first getMore (find)
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php NEEDS_CRYPTO(); ?>
<?php NEEDS('STANDALONE'); ?>
<?php NEEDS_ATLEAST_MONGODB_VERSION(STANDALONE, "3.6"); ?>
<?php CLEANUP(STANDALONE); ?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

$manager = new MongoDB\Driver\Manager(STANDALONE);

$bulk = new MongoDB\Driver\BulkWrite;
$bulk->insert(['_id' => 1]);
$bulk->insert(['_id' => 2]);
$bulk->insert(['_id' => 3]);
$manager->executeBulkWrite(NS, $bulk);

$query = new MongoDB\Driver\Query([], ['batchSize' => 2]);
$session = $manager->startSession();

$cursor = $manager->executeQuery(NS, $query, ['session' => $session]);

foreach ($cursor as $document) {
unset($session);
echo $document->_id, "\n";
}

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
1
2
3
===DONE===
42 changes: 42 additions & 0 deletions tests/cursor/bug1151-002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
--TEST--
PHPC-1151: Segfault if session unset before first getMore (aggregate)
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php NEEDS_CRYPTO(); ?>
<?php NEEDS('STANDALONE'); ?>
<?php NEEDS_ATLEAST_MONGODB_VERSION(STANDALONE, "3.6"); ?>
<?php CLEANUP(STANDALONE); ?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

$manager = new MongoDB\Driver\Manager(STANDALONE);

$bulk = new MongoDB\Driver\BulkWrite;
$bulk->insert(['_id' => 1]);
$bulk->insert(['_id' => 2]);
$bulk->insert(['_id' => 3]);
$manager->executeBulkWrite(NS, $bulk);

$command = new MongoDB\Driver\Command([
'aggregate' => COLLECTION_NAME,
'pipeline' => [],
'cursor' => ['batchSize' => 2],
]);
$session = $manager->startSession();

$cursor = $manager->executeReadCommand(DATABASE_NAME, $command, ['session' => $session]);

foreach ($cursor as $document) {
unset($session);
echo $document->_id, "\n";
}

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
1
2
3
===DONE===
32 changes: 32 additions & 0 deletions tests/cursor/bug1151-003.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
--TEST--
PHPC-1151: Segfault if session unset before cursor is killed (find)
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php NEEDS_CRYPTO(); ?>
<?php NEEDS('STANDALONE'); ?>
<?php NEEDS_ATLEAST_MONGODB_VERSION(STANDALONE, "3.6"); ?>
<?php CLEANUP(STANDALONE); ?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

$manager = new MongoDB\Driver\Manager(STANDALONE);

$bulk = new MongoDB\Driver\BulkWrite;
$bulk->insert(['_id' => 1]);
$bulk->insert(['_id' => 2]);
$bulk->insert(['_id' => 3]);
$manager->executeBulkWrite(NS, $bulk);

$query = new MongoDB\Driver\Query([], ['batchSize' => 2]);
$session = $manager->startSession();

$cursor = $manager->executeQuery(NS, $query, ['session' => $session]);
unset($session);
unset($cursor);

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
===DONE===
36 changes: 36 additions & 0 deletions tests/cursor/bug1151-004.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
--TEST--
PHPC-1151: Segfault if session unset before cursor is killed (aggregate)
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php NEEDS_CRYPTO(); ?>
<?php NEEDS('STANDALONE'); ?>
<?php NEEDS_ATLEAST_MONGODB_VERSION(STANDALONE, "3.6"); ?>
<?php CLEANUP(STANDALONE); ?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

$manager = new MongoDB\Driver\Manager(STANDALONE);

$bulk = new MongoDB\Driver\BulkWrite;
$bulk->insert(['_id' => 1]);
$bulk->insert(['_id' => 2]);
$bulk->insert(['_id' => 3]);
$manager->executeBulkWrite(NS, $bulk);

$command = new MongoDB\Driver\Command([
'aggregate' => COLLECTION_NAME,
'pipeline' => [],
'cursor' => ['batchSize' => 2],
]);
$session = $manager->startSession();

$cursor = $manager->executeReadCommand(DATABASE_NAME, $command, ['session' => $session]);
unset($session);
unset($cursor);

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
===DONE===
2 changes: 2 additions & 0 deletions tests/manager/manager-executeCommand-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) {
}
["readPreference"]=>
NULL
["session"]=>
NULL
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a test case where "session" is not NULL?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not presently. Would you like it added to this PR or should I just intend to do so for one of the tests in #803 for PHPC-1152?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think putting it in #803 makes more sense.

["isDead"]=>
bool(false)
["currentIndex"]=>
Expand Down
2 changes: 2 additions & 0 deletions tests/manager/manager-executeQuery-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) {
NULL
["readPreference"]=>
NULL
["session"]=>
NULL
["isDead"]=>
bool(false)
["currentIndex"]=>
Expand Down
2 changes: 2 additions & 0 deletions tests/manager/manager-executeQuery-002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) {
NULL
["readPreference"]=>
NULL
["session"]=>
NULL
["isDead"]=>
bool(false)
["currentIndex"]=>
Expand Down
10 changes: 10 additions & 0 deletions tests/readPreference/bug0146-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) {
["mode"]=>
string(7) "primary"
}
["session"]=>
NULL
["isDead"]=>
bool(true)
["currentIndex"]=>
Expand Down Expand Up @@ -96,6 +98,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) {
["mode"]=>
string(16) "primaryPreferred"
}
["session"]=>
NULL
["isDead"]=>
bool(true)
["currentIndex"]=>
Expand Down Expand Up @@ -134,6 +138,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) {
["mode"]=>
string(9) "secondary"
}
["session"]=>
NULL
["isDead"]=>
bool(true)
["currentIndex"]=>
Expand Down Expand Up @@ -172,6 +178,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) {
["mode"]=>
string(18) "secondaryPreferred"
}
["session"]=>
NULL
["isDead"]=>
bool(true)
["currentIndex"]=>
Expand Down Expand Up @@ -210,6 +218,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) {
["mode"]=>
string(7) "nearest"
}
["session"]=>
NULL
["isDead"]=>
bool(true)
["currentIndex"]=>
Expand Down
Loading