Skip to content

PHPC-1140: Implement Transactions specification #850

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.

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
Jun 19, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ matrix:
- php: 7.0
env:
- SERVER_VERSION=3.4.11
- php: 7.0
env:
- SERVER_VERSION=4.0.0-rc4
Copy link
Member

Choose a reason for hiding this comment

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

Relevant: #851

I'm not sure this will be helpful for the transaction tests (pending PHPC-1184), since they require a replica set oplog to exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I am aware of that. But some tests just need a 4.0.0 one to run at, as they don't actually talk to the Database yet. But libmongoc still does the version check with a single server.


before_install:
- pip install "mongo-orchestration>=0.6.7,<1.0" --user `whoami`
Expand Down
43 changes: 0 additions & 43 deletions Vagrantfile
Original file line number Diff line number Diff line change
Expand Up @@ -39,48 +39,5 @@ Vagrant.configure(2) do |config|
ldap.vm.provision "shell", path: "scripts/centos/essentials.sh", privileged: true
ldap.vm.provision "shell", path: "scripts/centos/ldap/install.sh", privileged: true
end

config.vm.define "freebsd", autostart: false do |bsd|
bsd.vm.network "private_network", ip: "192.168.112.30"

bsd.vm.box = "geoffgarside/freebsd-10.0"

bsd.vm.provision "shell", path: "scripts/freebsd/essentials.sh", privileged: true
bsd.vm.provision "file", source: "/tmp/PHONGO-SERVERS.json", destination: "/tmp/PHONGO-SERVERS.json"
bsd.vm.provision "file", source: "scripts/configs/.gdbinit", destination: "/home/vagrant/.gdbinit"
bsd.vm.provision "shell", path: "scripts/freebsd/phongo.sh", privileged: true
bsd.vm.synced_folder ".", "/phongo", :nfs => true, id: "vagrant-root"
end

config.vm.define "precise64" do |linux|
linux.vm.network "private_network", ip: "192.168.112.40"

linux.vm.box = "http://files.vagrantup.com/precise64.box"
linux.vm.provider "vmware_workstation" do |vmware, override|
override.vm.box_url = 'http://files.vagrantup.com/precise64_vmware.box'
override.vm.provision "shell", path: "scripts/vmware/kernel.sh", privileged: true
end

linux.vm.provision "shell", path: "scripts/ubuntu/essentials.sh", privileged: true
linux.vm.provision "file", source: "/tmp/PHONGO-SERVERS.json", destination: "/tmp/PHONGO-SERVERS.json"
linux.vm.provision "file", source: "scripts/configs/.gdbinit", destination: "/home/vagrant/.gdbinit"
linux.vm.provision "shell", path: "scripts/ubuntu/phongo.sh", privileged: true
end

config.vm.define "precise32" do |linux|
linux.vm.network "private_network", ip: "192.168.112.50"

linux.vm.box = "bjori/precise32"
linux.vm.provider "vmware_workstation" do |vmware, override|
override.vm.box_url = "bjori/precise32"
override.vm.provision "shell", path: "scripts/vmware/kernel.sh", privileged: true
end

linux.vm.provision "shell", path: "scripts/ubuntu/essentials.sh", privileged: true
linux.vm.provision "file", source: "/tmp/PHONGO-SERVERS.json", destination: "/tmp/PHONGO-SERVERS.json"
linux.vm.provision "file", source: "scripts/configs/.gdbinit", destination: "/home/vagrant/.gdbinit"
linux.vm.provision "shell", path: "scripts/ubuntu/phongo.sh", privileged: true
end

end

52 changes: 29 additions & 23 deletions php_phongo.c
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,39 @@ void phongo_throw_exception(php_phongo_error_domain_t domain TSRMLS_DC, const ch
efree(message);
va_end(args);
}

void phongo_throw_exception_from_bson_error_t(bson_error_t* error TSRMLS_DC)
{
zend_throw_exception(phongo_exception_from_mongoc_domain(error->domain, error->code), error->message, error->code TSRMLS_CC);
}

void phongo_throw_exception_from_bson_error_and_reply_t(bson_error_t* error, bson_t* reply TSRMLS_DC)
{
/* Server errors (other than ExceededTimeLimit) and write concern errors
* may use CommandException and report the result document for the
* failed command. For BC, ExceededTimeLimit errors will continue to use
* ExcecutionTimeoutException and omit the result document. */
if ((error->domain == MONGOC_ERROR_SERVER && error->code != PHONGO_SERVER_ERROR_EXCEEDED_TIME_LIMIT) || error->domain == MONGOC_ERROR_WRITE_CONCERN) {
#if PHP_VERSION_ID >= 70000
zval zv;
#else
zval* zv;
#endif

zend_throw_exception(php_phongo_commandexception_ce, error->message, error->code TSRMLS_CC);
php_phongo_bson_to_zval(bson_get_data(reply), reply->len, &zv);

#if PHP_VERSION_ID >= 70000
phongo_add_exception_prop(ZEND_STRL("resultDocument"), &zv);
#else
phongo_add_exception_prop(ZEND_STRL("resultDocument"), zv TSRMLS_CC);
#endif
zval_ptr_dtor(&zv);
} else {
phongo_throw_exception_from_bson_error_t(error TSRMLS_CC);
}
}

static void php_phongo_log(mongoc_log_level_t log_level, const char* log_domain, const char* message, void* user_data)
{
struct timeval tv;
Expand Down Expand Up @@ -927,29 +955,7 @@ bool phongo_execute_command(mongoc_client_t* client, php_phongo_command_type_t t
free_reply = true;

if (!result) {
/* Server errors (other than ExceededTimeLimit) and write concern errors
* may use CommandException and report the result document for the
* failed command. For BC, ExceededTimeLimit errors will continue to use
* ExcecutionTimeoutException and omit the result document. */
if ((error.domain == MONGOC_ERROR_SERVER && error.code != PHONGO_SERVER_ERROR_EXCEEDED_TIME_LIMIT) || error.domain == MONGOC_ERROR_WRITE_CONCERN) {
#if PHP_VERSION_ID >= 70000
zval zv;
#else
zval* zv;
#endif

zend_throw_exception(php_phongo_commandexception_ce, error.message, error.code TSRMLS_CC);
php_phongo_bson_to_zval(bson_get_data(&reply), reply.len, &zv);

#if PHP_VERSION_ID >= 70000
phongo_add_exception_prop(ZEND_STRL("resultDocument"), &zv);
#else
phongo_add_exception_prop(ZEND_STRL("resultDocument"), zv TSRMLS_CC);
#endif
zval_ptr_dtor(&zv);
} else {
phongo_throw_exception_from_bson_error_t(&error TSRMLS_CC);
}
phongo_throw_exception_from_bson_error_and_reply_t(&error, &reply TSRMLS_CC);
goto cleanup;
}

Expand Down
1 change: 1 addition & 0 deletions php_phongo.h
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ void phongo_throw_exception(php_phongo_error_domain_t domain TSRMLS
#endif /* PHP_VERSION_ID < 70000 */
;
void phongo_throw_exception_from_bson_error_t(bson_error_t* error TSRMLS_DC);
void phongo_throw_exception_from_bson_error_and_reply_t(bson_error_t* error, bson_t* reply TSRMLS_DC);

/* This enum is used for processing options in phongo_execute_parse_options and
* selecting a libmongoc function to use in phongo_execute_command. The values
Expand Down
3 changes: 3 additions & 0 deletions scripts/presets/replicaset-30.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
"noprealloc": true,
"nssize": 1,
"port": 3100,
"bind_ip": "0.0.0.0,::",
"smallfiles": true,
"setParameter": {"enableTestCommands": 1}
},
Expand All @@ -34,6 +35,7 @@
"noprealloc": true,
"nssize": 1,
"port": 3101,
"bind_ip": "0.0.0.0,::",
"smallfiles": true,
"setParameter": {"enableTestCommands": 1}
},
Expand All @@ -56,6 +58,7 @@
"noprealloc": true,
"nssize": 1,
"port": 3102,
"bind_ip": "0.0.0.0,::",
"smallfiles": true,
"setParameter": {"enableTestCommands": 1}
},
Expand Down
3 changes: 3 additions & 0 deletions scripts/presets/replicaset-dns.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
"noprealloc": true,
"nssize": 1,
"port": 27017,
"bind_ip_all": true,
"smallfiles": true,
"setParameter": {"enableTestCommands": 1}
},
Expand All @@ -30,6 +31,7 @@
"noprealloc": true,
"nssize": 1,
"port": 27018,
"bind_ip_all": true,
"smallfiles": true,
"setParameter": {"enableTestCommands": 1}
},
Expand All @@ -48,6 +50,7 @@
"noprealloc": true,
"nssize": 1,
"port": 27019,
"bind_ip_all": true,
"smallfiles": true,
"setParameter": {"enableTestCommands": 1}
},
Expand Down
3 changes: 3 additions & 0 deletions scripts/presets/replicaset.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
"noprealloc": true,
"nssize": 1,
"port": 3000,
"bind_ip_all": true,
"smallfiles": true,
"setParameter": {"enableTestCommands": 1}
},
Expand All @@ -34,6 +35,7 @@
"noprealloc": true,
"nssize": 1,
"port": 3001,
"bind_ip_all": true,
"smallfiles": true,
"setParameter": {"enableTestCommands": 1}
},
Expand All @@ -56,6 +58,7 @@
"noprealloc": true,
"nssize": 1,
"port": 3002,
"bind_ip_all": true,
"smallfiles": true,
"setParameter": {"enableTestCommands": 1}
},
Expand Down
1 change: 1 addition & 0 deletions scripts/presets/standalone-30.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"logpath": "/tmp/standalone-30/mongod.log",
"journal": true,
"port": 2700,
"bind_ip": "0.0.0.0,::",
"setParameter": {"enableTestCommands": 1}
},
"version": "30-release"
Expand Down
1 change: 1 addition & 0 deletions scripts/presets/standalone-auth.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"logpath": "/tmp/standalone-auth/m.log",
"journal": true,
"port": 2200,
"bind_ip_all": true,
"setParameter": {"enableTestCommands": 1}
}
}
Expand Down
3 changes: 2 additions & 1 deletion scripts/presets/standalone-plain.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
"logpath": "/tmp/standalone-plain/m.log",
"journal": true,
"port": 2400,
"setParameter": {"enableTestCommands": 1, "saslauthdPath": "/var/run/saslauthd/mux", "authenticationMechanisms": "MONGODB-CR,PLAIN"}
"bind_ip_all": true,
"setParameter": {"enableTestCommands": 1, "saslauthdPath": "/var/run/saslauthd/mux", "authenticationMechanisms": "SCRAM-SHA-1,PLAIN"}
}
}

1 change: 1 addition & 0 deletions scripts/presets/standalone-ssl.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"logpath": "/tmp/standalone-ssl/m.log",
"journal": true,
"port": 2100,
"bind_ip_all": true,
"setParameter": {"enableTestCommands": 1}
},
"sslParams": {
Expand Down
1 change: 1 addition & 0 deletions scripts/presets/standalone-x509.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"logpath": "/tmp/standalone-x509/m.log",
"journal": true,
"port": 2300,
"bind_ip_all": true,
"setParameter": {"enableTestCommands": 1, "authenticationMechanisms": "MONGODB-X509"}
},
"sslParams": {
Expand Down
1 change: 1 addition & 0 deletions scripts/presets/standalone.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"logpath": "/tmp/standalone/mongod.log",
"journal": true,
"port": 2000,
"bind_ip_all": true,
"setParameter": {"enableTestCommands": 1}
}
}
Expand Down
1 change: 1 addition & 0 deletions scripts/ubuntu/mongo-orchestration-config.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{
"releases": {
"40-release": "/home/vagrant/4.0/usr/bin",
"36-release": "/home/vagrant/3.6/usr/bin",
Copy link
Member

@jmikola jmikola Jun 18, 2018

Choose a reason for hiding this comment

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

I don't think anything is using this. It was only important as the first version listed under "releases", which made it the default. Now that "40-release" is here, I think we can just remove this.

No objections if you want to keep it around, though. scripts/ubuntu/mongo-orchestration.sh is still installing it, so there's no harm in leaving this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll leave it for now.

"30-release": "/home/vagrant/3.0/usr/bin"
}
Expand Down
6 changes: 6 additions & 0 deletions scripts/ubuntu/mongo-orchestration.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,12 @@ apt-key adv --keyserver hkp://keyserver.ubuntu.com:80 --recv 7F0CEB10
# 3.6
apt-key adv --keyserver hkp://keyserver.ubuntu.com:80 --recv 2930ADAE8CAF5059EE73BB4B58712A2291FA4AD5

# 4.0
apt-key adv --keyserver hkp://keyserver.ubuntu.com:80 --recv 68818C72E52529D4

echo 'deb http://repo.mongodb.com/apt/ubuntu trusty/mongodb-enterprise/3.0 multiverse' | tee /etc/apt/sources.list.d/mongodb-enterprise-3.0.list
echo 'deb http://repo.mongodb.com/apt/ubuntu trusty/mongodb-enterprise/3.6 multiverse' | tee /etc/apt/sources.list.d/mongodb-enterprise-3.6.list
echo 'deb http://repo.mongodb.com/apt/ubuntu trusty/mongodb-enterprise/testing multiverse' | tee /etc/apt/sources.list.d/mongodb-enterprise-4.0.list

apt-get update

Expand All @@ -14,9 +18,11 @@ apt-get install -y libsnmp30 libgsasl7 libcurl4-openssl-dev
apt-get download mongodb-enterprise-server=3.0.15
apt-get download mongodb-enterprise-server=3.6.1
apt-get download mongodb-enterprise-mongos=3.6.1
apt-get download mongodb-enterprise-server=4.0.0~rc5
dpkg -x mongodb-enterprise-server_3.0.15_amd64.deb 3.0
dpkg -x mongodb-enterprise-server_3.6.1_amd64.deb 3.6
dpkg -x mongodb-enterprise-mongos_3.6.1_amd64.deb 3.6
dpkg -x mongodb-enterprise-server_4.0.0~rc5_amd64.deb 4.0

# Python stuff for mongo-orchestration
apt-get install -y python python-dev
Expand Down
54 changes: 45 additions & 9 deletions src/MongoDB/Manager.c
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "php_array_api.h"
#include "phongo_compat.h"
#include "php_phongo.h"
#include "Session.h"

#define PHONGO_MANAGER_URI_DEFAULT "mongodb://127.0.0.1/"

Expand Down Expand Up @@ -655,11 +656,12 @@ static PHP_METHOD(Manager, selectServer)
Returns a new client session */
static PHP_METHOD(Manager, startSession)
{
php_phongo_manager_t* intern;
zval* options = NULL;
mongoc_session_opt_t* cs_opts = NULL;
mongoc_client_session_t* cs;
bson_error_t error = { 0 };
php_phongo_manager_t* intern;
zval* options = NULL;
mongoc_session_opt_t* cs_opts = NULL;
mongoc_client_session_t* cs;
bson_error_t error = { 0 };
mongoc_transaction_opt_t* txn_opts = NULL;
SUPPRESS_UNUSED_WARNING(return_value_ptr)
SUPPRESS_UNUSED_WARNING(return_value_used)

Expand All @@ -669,22 +671,56 @@ static PHP_METHOD(Manager, startSession)
return;
}

if (options && php_array_exists(options, "causalConsistency")) {
if (options && php_array_existsc(options, "causalConsistency")) {
Copy link
Member

Choose a reason for hiding this comment

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

IIRC this ensures we use sizeof() instead of strlen(). Good catch 👍

cs_opts = mongoc_session_opts_new();
mongoc_session_opts_set_causal_consistency(cs_opts, php_array_fetchc_bool(options, "causalConsistency"));
}

cs = mongoc_client_start_session(intern->client, cs_opts, &error);
if (options && php_array_existsc(options, "defaultTransactionOptions")) {
zval* txn_options = php_array_fetchc(options, "defaultTransactionOptions");

if (cs_opts) {
mongoc_session_opts_destroy(cs_opts);
/* Thrown exception and return if the defaultTransactionOptions is not an array */
if (Z_TYPE_P(txn_options) != IS_ARRAY) {
phongo_throw_exception(
PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC,
"Expected \"defaultTransactionOptions\" option to be an array, %s given",
PHONGO_ZVAL_CLASS_OR_TYPE_NAME_P(txn_options)
);
goto cleanup;
}

/* Parse transaction options */
txn_opts = php_mongodb_session_parse_transaction_options(txn_options TSRMLS_CC);

/* If an exception is thrown while parsing, the txn_opts struct is also
* NULL, so no need to free it here */
if (EG(exception)) {
goto cleanup;
}

/* If the options are non-empty, add them to the client session opts struct */
if (txn_opts) {
if (!cs_opts) {
cs_opts = mongoc_session_opts_new();
}

mongoc_session_opts_set_default_transaction_opts(cs_opts, txn_opts);
Copy link
Member

Choose a reason for hiding this comment

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

Can we call mongoc_transaction_opts_destroy() after this? Ideally, this should be the only point where txn_opts is non-NULL and in need of freeing (provided the memory leak in https://github.com/mongodb/mongo-php-driver/pull/850/files#r196198256 is fixed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, done. And updated relevant comments.

mongoc_transaction_opts_destroy(txn_opts);
}
}

cs = mongoc_client_start_session(intern->client, cs_opts, &error);

if (cs) {
phongo_session_init(return_value, cs TSRMLS_CC);
} else {
phongo_throw_exception_from_bson_error_t(&error TSRMLS_CC);
}

cleanup:
if (cs_opts) {
mongoc_session_opts_destroy(cs_opts);
}
} /* }}} */

/* {{{ MongoDB\Driver\Manager function entries */
Expand Down
Loading