Skip to content
This repository was archived by the owner on Feb 1, 2022. It is now read-only.

Fixed transaction issues with GridFS #45

Merged
merged 13 commits into from
Feb 22, 2012
Merged

Fixed transaction issues with GridFS #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0b128a5
Added a test case for PHP-310
derickr Feb 9, 2012
bd1b24e
Fixed some more memory leaks with GridFS.
derickr Feb 9, 2012
a9420d8
Allow GridFS files to be deleted by any type of ID.
derickr Feb 16, 2012
d868854
Fixed PHP-310: reverting with GridFS.
derickr Feb 16, 2012
73b657a
Update test case. In order for this to work, you need safe mode.
derickr Feb 16, 2012
fa6d707
Force safe-mode for the meta-record of gridfs.
derickr Feb 18, 2012
04e6a5c
Rename to correct bug number.
derickr Feb 18, 2012
99b7163
Add expected result to test.
derickr Feb 18, 2012
5bbf4b0
Fixed a memory leak when not freeing a cursor.
derickr Feb 18, 2012
b31f07b
Also implement the cleanups for storeBytes
derickr Feb 18, 2012
472026c
Added a test case for storeBytes() case as well.
derickr Feb 18, 2012
dc98d5f
Added a test case for GridFS::delete() with random types for the _id …
derickr Feb 20, 2012
bc6e7c5
Added splitting of the options array as we're modifying it, and make …
derickr Feb 22, 2012
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
241 changes: 181 additions & 60 deletions gridfs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
#include "mongo_types.h"
#include "db.h"

#include "ext/standard/php_smart_str.h"

typedef struct {
FILE *file;
int fd; /* underlying file descriptor */
Expand Down Expand Up @@ -335,6 +337,57 @@ static void add_md5(zval *zfile, zval *zid, mongo_collection *c TSRMLS_DC) {
}
}

static cleanup_broken_insert(INTERNAL_FUNCTION_PARAMETERS, zval *zid TSRMLS_CC)
{
zval *chunks, *files, *criteria_chunks, *criteria_files;
char *message = NULL;
smart_str tmp_message = { 0 };
zval *temp_return;

chunks = zend_read_property(mongo_ce_GridFS, getThis(), "chunks", strlen("chunks"), NOISY TSRMLS_CC);
if (EG(exception)) {
message = estrdup(Z_STRVAL_P(zend_read_property(mongo_ce_GridFSException, EG(exception), "message", strlen("message"), NOISY TSRMLS_CC)));
zend_clear_exception(TSRMLS_C);
}

MAKE_STD_ZVAL(criteria_files);
array_init(criteria_files);
zval_add_ref(&zid);
add_assoc_zval(criteria_files, "_id", zid);

MAKE_STD_ZVAL(criteria_chunks);
array_init(criteria_chunks);
zval_add_ref(&zid);
add_assoc_zval(criteria_chunks, "files_id", zid);

MAKE_STD_ZVAL(temp_return);
ZVAL_NULL(temp_return);
MONGO_METHOD1(MongoCollection, remove, temp_return, chunks, criteria_chunks);
zval_ptr_dtor(&temp_return);

MAKE_STD_ZVAL(temp_return);
ZVAL_NULL(temp_return);
MONGO_METHOD1(MongoCollection, remove, temp_return, getThis(), criteria_files);
zval_ptr_dtor(&temp_return);

zval_ptr_dtor(&criteria_files);
zval_ptr_dtor(&criteria_chunks);

// create the message for the exception
if (message) {
smart_str_appends(&tmp_message, "Could not store file: ");
smart_str_appends(&tmp_message, message);
smart_str_0(&tmp_message);
efree(message);
} else {
smart_str_appends(&tmp_message, "Could not store file for unknown reasons");
smart_str_0(&tmp_message);
}
zend_throw_exception(mongo_ce_GridFSException, tmp_message.c, 0 TSRMLS_CC);
smart_str_free(&tmp_message);
RETVAL_FALSE;
}

/*
* Stores an array of bytes that may not have a filename,
* such as data from a socket or stream.
Expand All @@ -348,6 +401,7 @@ PHP_METHOD(MongoGridFS, storeBytes) {
char *bytes = 0;
int bytes_len = 0, chunk_num = 0, chunk_size = 0, global_chunk_size = 0,
pos = 0;
int free_options = 0, revert = 0;

zval temp;
zval *extra = 0, *zid = 0, *zfile = 0, *chunks = 0, *options = 0;
Expand All @@ -358,16 +412,9 @@ PHP_METHOD(MongoGridFS, storeBytes) {
chunks = zend_read_property(mongo_ce_GridFS, getThis(), "chunks", strlen("chunks"), NOISY TSRMLS_CC);
ensure_gridfs_index(&temp, chunks TSRMLS_CC);

if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s|az", &bytes, &bytes_len, &extra, &options) == FAILURE) {
return;
}

if (!options) {
zval *opts;
MAKE_STD_ZVAL(opts);
array_init(opts);
options = opts;
}
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s|aa/", &bytes, &bytes_len, &extra, &options) == FAILURE) {
return;
}

// file array object
MAKE_STD_ZVAL(zfile);
Expand All @@ -383,14 +430,30 @@ PHP_METHOD(MongoGridFS, storeBytes) {
add_assoc_long(zfile, "length", bytes_len);
}

// options
if (!options) {
zval *opts;
MAKE_STD_ZVAL(opts);
array_init(opts);
options = opts;
free_options = 1;
}

// force safe mode
add_assoc_long(options, "safe", 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you need to separate the zval to not modify the original array here?

Infact.. I don't see where the input argument is ever forced as array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Mon, 20 Feb 2012, Hannes Magnusson wrote:

@@ -383,14 +430,30 @@ static void add_md5(zval *zfile, zval *zid, mongo_collection *c TSRMLS_DC) {
add_assoc_long(zfile, "length", bytes_len);
}

  • // options
  • if (!options) {
  •   zval *opts;

  •   MAKE_STD_ZVAL(opts);

  •   array_init(opts);

  •   options = opts;

  •   free_options = 1;
  • }
  • // force safe mode
  • add_assoc_long(options, "safe", 1);

Don't you need to separate the zval to not modify the original array here?

Hmm, good point :-)

Infact.. I don't see where the input argument is ever forced as array?

Also a good point. I noticed that in a lot of cases just a "z" is used
and not the actual. In this case however, changing "s|az" tp "s|as"
would be correct.

cheers,
Derick

http://derickrethans.nl | http://xdebug.org
Like Xdebug? Consider a donation: http://xdebug.org/donate.php
twitter: @derickr and @xdebug


// insert chunks
while (pos < bytes_len) {
chunk_size = bytes_len-pos >= global_chunk_size ? global_chunk_size : bytes_len-pos;

insert_chunk(chunks, zid, chunk_num, bytes+pos, chunk_size, options TSRMLS_CC);
if (EG(exception)) {
return;
}
if (insert_chunk(chunks, zid, chunk_num, bytes+pos, chunk_size, options TSRMLS_CC) == FAILURE) {
revert = 1;
goto cleanup_on_failure;
}
if (EG(exception)) {
revert = 1;
goto cleanup_on_failure;
}

// increment counters
pos += chunk_size;
Expand All @@ -402,11 +465,25 @@ PHP_METHOD(MongoGridFS, storeBytes) {

// insert file
MONGO_METHOD2(MongoCollection, insert, &temp, getThis(), zfile, options);

zval_add_ref(&zid);
zval_ptr_dtor(&zfile);

RETURN_ZVAL(zid, 1, 1);
zval_dtor(&temp);
if (EG(exception)) {
revert = 1;
}

cleanup_on_failure:
if (!revert) {
RETVAL_ZVAL(zid, 1, 1);
} else {
cleanup_broken_insert(INTERNAL_FUNCTION_PARAM_PASSTHRU, zid TSRMLS_CC);
RETVAL_FALSE;
}

zval_add_ref(&zid);
zval_ptr_dtor(&zfile);

if (free_options) {
zval_ptr_dtor(&options);
}
}

/* add extra fields required for files:
Expand Down Expand Up @@ -454,7 +531,7 @@ static int setup_file_fields(zval *zfile, char *filename, int size TSRMLS_DC) {
* - buf
*/
static int insert_chunk(zval *chunks, zval *zid, int chunk_num, char *buf, int chunk_size, zval *options TSRMLS_DC) {
zval temp;
zval temp;
zval *zchunk, *zbin;

// create chunk
Expand All @@ -480,6 +557,7 @@ static int insert_chunk(zval *chunks, zval *zid, int chunk_num, char *buf, int c
else {
MONGO_METHOD1(MongoCollection, insert, &temp, chunks, zchunk);
}
zval_dtor(&temp);

// increment counters
zval_ptr_dtor(&zchunk); // zid->refcount = 1
Expand All @@ -496,6 +574,7 @@ PHP_METHOD(MongoGridFS, storeFile) {
zval *fh, *extra = 0, *options = 0;
char *filename = 0;
int chunk_num = 0, global_chunk_size = 0, size = 0, pos = 0, fd = -1, safe = 0;
int free_options = 0, revert = 0;
FILE *fp = 0;

zval temp;
Expand All @@ -507,16 +586,9 @@ PHP_METHOD(MongoGridFS, storeFile) {

ensure_gridfs_index(&temp, chunks TSRMLS_CC);

if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "z|az", &fh, &extra, &options) == FAILURE) {
return;
}

if (!options) {
zval *opts;
MAKE_STD_ZVAL(opts);
array_init(opts);
options = opts;
}
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "z|aa/", &fh, &extra, &options) == FAILURE) {
return;
}

if (Z_TYPE_P(fh) == IS_RESOURCE) {
zend_rsrc_list_entry *le;
Expand Down Expand Up @@ -577,6 +649,18 @@ PHP_METHOD(MongoGridFS, storeFile) {
// chunkSize
global_chunk_size = get_chunk_size(zfile TSRMLS_CC);

// options
if (!options) {
zval *opts;
MAKE_STD_ZVAL(opts);
array_init(opts);
options = opts;
free_options = 1;
}

// force safe mode
add_assoc_long(options, "safe", 1);

// insert chunks
while (pos < size || fp == 0) {
int result = 0;
Expand All @@ -588,32 +672,41 @@ PHP_METHOD(MongoGridFS, storeFile) {
if (fp) {
if ((int)fread(buf, 1, chunk_size, fp) < chunk_size) {
zend_throw_exception_ex(mongo_ce_GridFSException, 0 TSRMLS_CC, "error reading file %s", filename);
return;
revert = 1;
efree(buf);
goto cleanup_on_failure;
}
pos += chunk_size;
if (insert_chunk(chunks, zid, chunk_num, buf, chunk_size, options TSRMLS_CC) == FAILURE) {
break;
revert = 1;
efree(buf);
goto cleanup_on_failure;
}
}
else {
result = read(fd, buf, chunk_size);
if (result == -1) {
zend_throw_exception_ex(mongo_ce_GridFSException, 0 TSRMLS_CC, "error reading filehandle");
return;
revert = 1;
efree(buf);
goto cleanup_on_failure;
}
pos += result;
if (insert_chunk(chunks, zid, chunk_num, buf, result, options TSRMLS_CC) == FAILURE) {
break;
revert = 1;
efree(buf);
goto cleanup_on_failure;
}
}

if (safe && EG(exception)) {
return;
}
efree(buf);

chunk_num++;
if (safe && EG(exception)) {
revert = 1;
goto cleanup_on_failure;
}

efree(buf);
chunk_num++;

if (fp == 0 && result < chunk_size) {
break;
Expand All @@ -626,8 +719,7 @@ PHP_METHOD(MongoGridFS, storeFile) {
}

if (EG(exception)) {
zval_ptr_dtor(&zfile);
return;
goto cleanup_on_failure;
}

if (!fp) {
Expand All @@ -636,14 +728,39 @@ PHP_METHOD(MongoGridFS, storeFile) {

add_md5(zfile, zid, c TSRMLS_CC);

// insert file
MONGO_METHOD2(MongoCollection, insert, &temp, getThis(), zfile, options);

// cleanup
zval_add_ref(&zid);
zval_ptr_dtor(&zfile);

RETURN_ZVAL(zid, 1, 1);
// insert file
if (!revert) {
zval *temp_return;

Z_ADDREF_P(options);
MAKE_STD_ZVAL(temp_return);
ZVAL_NULL(temp_return);
MONGO_METHOD2(MongoCollection, insert, temp_return, getThis(), zfile, options);
zval_ptr_dtor(&temp_return);
Z_DELREF_P(options);
if (EG(exception)) {
revert = 1;
}
}

if (!revert) {
RETVAL_ZVAL(zid, 1, 1);
}

cleanup_on_failure:
// remove all inserted chunks and main file document
if (revert) {
cleanup_broken_insert(INTERNAL_FUNCTION_PARAM_PASSTHRU, zid TSRMLS_DC);
RETVAL_FALSE;
}

// cleanup
zval_add_ref(&zid);
zval_ptr_dtor(&zfile);

if (free_options) {
zval_ptr_dtor(&options);
}
}

PHP_METHOD(MongoGridFS, findOne) {
Expand Down Expand Up @@ -826,21 +943,25 @@ PHP_METHOD(MongoGridFS, storeUpload) {
* New GridFS API
*/

PHP_METHOD(MongoGridFS, delete) {
zval *id, *criteria;
PHP_METHOD(MongoGridFS, delete)
{
zval *id, *criteria;
char *str_id;
size_t str_len;

if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "O", &id, mongo_ce_Id) == FAILURE) {
return;
}
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "z", &id) == FAILURE) {
return;
}

MAKE_STD_ZVAL(criteria);
array_init(criteria);
add_assoc_zval(criteria, "_id", id);
zval_add_ref(&id);
// Set up criteria array
MAKE_STD_ZVAL(criteria);
array_init(criteria);
add_assoc_zval(criteria, "_id", id);
zval_add_ref(&id);

MONGO_METHOD1(MongoGridFS, remove, return_value, getThis(), criteria);
MONGO_METHOD1(MongoGridFS, remove, return_value, getThis(), criteria);

zval_ptr_dtor(&criteria);
zval_ptr_dtor(&criteria);
}

PHP_METHOD(MongoGridFS, get) {
Expand Down
Loading