Skip to content

PHPC-1050: Command cursor should not invoke getMore at execution #730

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 1 commit into from
Jan 8, 2018
Merged

PHPC-1050: Command cursor should not invoke getMore at execution #730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 11 additions & 13 deletions php_phongo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ static void phongo_cursor_init(zval *return_value, mongoc_client_t *client, mong
intern->cursor = cursor;
intern->server_id = mongoc_cursor_get_hint(cursor);
intern->client = client;
intern->advanced = false;

if (readPreference) {
#if PHP_VERSION_ID >= 70000
Expand Down Expand Up @@ -286,6 +287,10 @@ static void phongo_cursor_init_for_query(zval *return_value, mongoc_client_t *cl
/* namespace has already been validated by phongo_execute_query() */
phongo_split_namespace(namespace, &intern->database, &intern->collection);

/* cursor has already been advanced by phongo_execute_query() calling
* phongo_cursor_advance_and_check_for_error() */
intern->advanced = true;

#if PHP_VERSION_ID >= 70000
ZVAL_ZVAL(&intern->query, query, 1, 0);
#else
Expand Down Expand Up @@ -709,9 +714,9 @@ bool phongo_execute_bulk_write(mongoc_client_t *client, const char *namespace, p
return success;
} /* }}} */

/* Advance the cursor and return whether there is an error. On error, the cursor
* will be destroyed and an exception will be thrown. */
static bool phongo_advance_cursor_and_check_for_error(mongoc_cursor_t *cursor TSRMLS_DC)
/* Advance the cursor and return whether there is an error. On error, false is
* returned and an exception is thrown. */
bool phongo_cursor_advance_and_check_for_error(mongoc_cursor_t *cursor TSRMLS_DC) /* {{{ */
{
const bson_t *doc;

Expand All @@ -720,20 +725,18 @@ static bool phongo_advance_cursor_and_check_for_error(mongoc_cursor_t *cursor TS

/* Check for connection related exceptions */
if (EG(exception)) {
mongoc_cursor_destroy(cursor);
return false;
}

/* Could simply be no docs, which is not an error */
if (mongoc_cursor_error(cursor, &error)) {
phongo_throw_exception_from_bson_error_t(&error TSRMLS_CC);
mongoc_cursor_destroy(cursor);
return false;
}
}

return true;
}
} /* }}} */

int phongo_execute_query(mongoc_client_t *client, const char *namespace, zval *zquery, zval *options, uint32_t server_id, zval *return_value, int return_value_used TSRMLS_DC) /* {{{ */
{
Expand Down Expand Up @@ -784,7 +787,8 @@ int phongo_execute_query(mongoc_client_t *client, const char *namespace, zval *z
mongoc_cursor_set_max_await_time_ms(cursor, query->max_await_time_ms);
}

if (!phongo_advance_cursor_and_check_for_error(cursor TSRMLS_CC)) {
if (!phongo_cursor_advance_and_check_for_error(cursor TSRMLS_CC)) {
mongoc_cursor_destroy(cursor);
Copy link
Contributor

Choose a reason for hiding this comment

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

It should not destroy the cursor, as phongo_cursor_advance_and_check_for_error already does that.

Copy link
Member Author

Choose a reason for hiding this comment

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

phongo_cursor_advance_and_check_for_error() no longer destroys the cursor. That change was made because we don't want to destroy it from within the iterator handlers.

return false;
}

Expand Down Expand Up @@ -912,15 +916,9 @@ int phongo_execute_command(mongoc_client_t *client, php_phongo_command_type_t ty
bson_destroy(&reply);
}

if (!phongo_advance_cursor_and_check_for_error(cmd_cursor TSRMLS_CC)) {
/* If an error is found, the cmd_cursor is destroyed already */
return false;
}

phongo_cursor_init_for_command(return_value, client, cmd_cursor, db, zcommand, zreadPreference TSRMLS_CC);
return true;
} /* }}} */

/* }}} */

/* {{{ mongoc types from from_zval */
Expand Down
2 changes: 2 additions & 0 deletions php_phongo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,8 @@ bool phongo_execute_bulk_write (mongoc_client_t *client, c
int phongo_execute_command (mongoc_client_t *client, php_phongo_command_type_t type, const char *db, zval *zcommand, zval *zreadPreference, uint32_t server_id, zval *return_value, int return_value_used TSRMLS_DC);
int phongo_execute_query (mongoc_client_t *client, const char *namespace, zval *zquery, zval *zreadPreference, uint32_t server_id, zval *return_value, int return_value_used TSRMLS_DC);

bool phongo_cursor_advance_and_check_for_error(mongoc_cursor_t *cursor TSRMLS_DC);

const mongoc_read_concern_t* phongo_read_concern_from_zval (zval *zread_concern TSRMLS_DC);
const mongoc_read_prefs_t* phongo_read_preference_from_zval(zval *zread_preference TSRMLS_DC);
const mongoc_write_concern_t* phongo_write_concern_from_zval (zval *zwrite_concern TSRMLS_DC);
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 @@ -55,6 +55,7 @@ typedef struct {
mongoc_cursor_t *cursor;
mongoc_client_t *client;
uint32_t server_id;
bool advanced;
php_phongo_bson_state visitor_data;
int got_iterator;
long current;
Expand Down
20 changes: 19 additions & 1 deletion src/MongoDB/Cursor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,15 @@ static void php_phongo_cursor_iterator_move_forward(zend_object_iterator *iter T
const bson_t *doc;

php_phongo_cursor_free_current(cursor);
cursor->current++;

/* If the cursor has already advanced, increment its position. Otherwise,
* the first call to mongoc_cursor_next() will be made below and we should
* leave its position at zero. */
if (cursor->advanced) {
cursor->current++;
} else {
cursor->advanced = true;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Future improvement: refactor phongo_cursor_advance_and_check_for_error() so that it can take an optional bson_t *. Then phongo_execute_query() and php_phongo_cursor_iterator_rewind() could pass in null, since they only care about error checking and don't need to access the document. Meanwhile, php_phongo_cursor_iterator_move_forward() could pass in a bson_t * and, on success, use that to populate the current element, as is done with:

php_phongo_bson_to_zval_ex(bson_get_data(doc), doc->len, &cursor->visitor_data);


if (mongoc_cursor_next(cursor->cursor, &doc)) {
php_phongo_bson_to_zval_ex(bson_get_data(doc), doc->len, &cursor->visitor_data);
Expand All @@ -117,6 +125,16 @@ static void php_phongo_cursor_iterator_rewind(zend_object_iterator *iter TSRMLS_
php_phongo_cursor_t *cursor = cursor_it->cursor;
const bson_t *doc;

/* If the cursor was never advanced (e.g. command cursor), do so now */
if (!cursor->advanced) {
cursor->advanced = true;

if (!phongo_cursor_advance_and_check_for_error(cursor->cursor TSRMLS_CC)) {
/* Exception should already have been thrown */
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but shouldn't you destroy the cursor now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Once we create a MongoDB\Driver\Cursor with the mongoc_cursor_t, the only place we destroy the libmongoc cursor is from php_phongo_cursor_free_object().

return;
}
}

if (cursor->current > 0) {
phongo_throw_exception(PHONGO_ERROR_LOGIC TSRMLS_CC, "Cursors cannot rewind after starting iteration");
return;
Expand Down
71 changes: 71 additions & 0 deletions tests/cursor/bug1050-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
--TEST--
PHPC-1050: Command cursor should not invoke getMore at execution
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php NEEDS('REPLICASET'); CLEANUP(REPLICASET); ?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

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

$cmd = new MongoDB\Driver\Command(
[
'aggregate' => COLLECTION_NAME,
'pipeline' => [
['$changeStream' => (object) []],
],
'cursor' => (object) [],
],
[
'maxAwaitTimeMS' => 1000,
]
);

$start = microtime(true);
$cursor = $manager->executeReadCommand(DATABASE_NAME, $cmd);
printf("Executing command took %0.6f seconds\n", microtime(true) - $start);

$it = new IteratorIterator($cursor);

$start = microtime(true);
$it->rewind();
printf("Rewinding cursor took %0.6f seconds\n", microtime(true) - $start);
printf("Current position is valid: %s\n", $it->valid() ? 'yes' : 'no');

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

$start = microtime(true);
$it->next();
printf("Advancing cursor took %0.6f seconds\n", microtime(true) - $start);
printf("Current position is valid: %s\n", $it->valid() ? 'yes' : 'no');

$document = $it->current();

if (isset($document)) {
printf("Operation type: %s\n", $document->operationType);
var_dump($document->fullDocument);
}

?>
===DONE===
<?php exit(0); ?>
--EXPECTF--
Executing command took 0.%d seconds
Rewinding cursor took 1.%d seconds
Current position is valid: no
Advancing cursor took %d.%d seconds
Current position is valid: yes
Operation type: insert
object(stdClass)#%d (%d) {
["_id"]=>
object(MongoDB\BSON\ObjectId)#%d (%d) {
["oid"]=>
string(24) "%x"
}
["x"]=>
int(1)
}
===DONE===
74 changes: 74 additions & 0 deletions tests/cursor/bug1050-002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
--TEST--
PHPC-1050: Command cursor should not invoke getMore at execution (rewind omitted)
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php NEEDS('REPLICASET'); CLEANUP(REPLICASET); ?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

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

$cmd = new MongoDB\Driver\Command(
[
'aggregate' => COLLECTION_NAME,
'pipeline' => [
['$changeStream' => (object) []],
],
'cursor' => (object) [],
],
[
'maxAwaitTimeMS' => 1000,
]
);

$start = microtime(true);
$cursor = $manager->executeReadCommand(DATABASE_NAME, $cmd);
printf("Executing command took %0.6f seconds\n", microtime(true) - $start);

$it = new IteratorIterator($cursor);

printf("Current position is valid: %s\n", $it->valid() ? 'yes' : 'no');

$start = microtime(true);
$it->next();
printf("Advancing cursor took %0.6f seconds\n", microtime(true) - $start);
Copy link
Member Author

Choose a reason for hiding this comment

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

Unlike the first test, this one omits the initial call to rewind() and tests that we preserve existing behavior by not requiring the rewind handler to ever be invoked (only relevant when wrapping a cursor with IteratorIterator).

printf("Current position is valid: %s\n", $it->valid() ? 'yes' : 'no');

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

$start = microtime(true);
$it->next();
printf("Advancing cursor took %0.6f seconds\n", microtime(true) - $start);
printf("Current position is valid: %s\n", $it->valid() ? 'yes' : 'no');

$document = $it->current();

if (isset($document)) {
printf("Operation type: %s\n", $document->operationType);
var_dump($document->fullDocument);
}

?>
===DONE===
<?php exit(0); ?>
--EXPECTF--
Executing command took 0.%d seconds
Current position is valid: no
Advancing cursor took 1.%d seconds
Current position is valid: no
Advancing cursor took %d.%d seconds
Current position is valid: yes
Operation type: insert
object(stdClass)#%d (%d) {
["_id"]=>
object(MongoDB\BSON\ObjectId)#%d (%d) {
["oid"]=>
string(24) "%x"
}
["x"]=>
int(1)
}
===DONE===