PHPC-433: Persist topology state between requests

[jmikola]

https://jira.mongodb.org/browse/PHPC-433
https://jira.mongodb.org/browse/PHPC-605
https://jira.mongodb.org/browse/PHPC-704

Depends on #361.

[jmikola]

The writeconcernerror->message and writeerror->message assignments were longstanding bugs, which weren't apparent until I tested with libbson's default memory allocation. These strings are both freed with efree() in the respective class dtors.

[bjori]

I don't understand the bug... nor the fix.

http://mongoc.org/libbson/current/bson_iter_dup_utf8.html

Yes, this string is supposed to be free()d with bson_free (), but that is the same as efree() because we set bson allocators to the same functions.

If phongo no longer sets custom bson allocator functions, wouldn't it be easier to keep this as is, but use bson_free () correctly, rather then bulky code to copy it?

[jmikola]

The bug was using inconsistent strdup() and free() functions, and the alternative fix was to use bson_free() in the class dtors. For consistency with other class structs, where the strings are always allocated by PHP, I chose to modify the allocation to match and leave the efree() in place.

Of course, the issue only presented itself when I disabled the custom BSON memory allocators, but I was uncomfortable with having this bit of unrelated code depend on that implementation detail.

[jmikola]

I'll note that I manually tested this PR with PHP's built-in web server (php -S localhost:8080) and watched as multiple requests utilized the same mongod connection. The index.php file I used was quite basic:

$manager = new MongoDB\Driver\Manager;
$cursor = $manager->executeCommand('test', new MongoDB\Driver\Command(['ping' => 1]));
var_dump($cursor->toArray()[0]);

This at least demonstrates that connections are persisted beyond RSHUTDOWN. When killing the web server, I can observe the connection being closed in the mongod server logs.

[bjori]

Is it intentional that the errormessage is so broad, rather then mentioning the exact option and the exact implementation?

[bjori]

Note that crl_file is also not supported by secure transport, nor libressl's libtls

[jmikola]

Is it intentional that the errormessage is so broad, rather then mentioning the exact option and the exact implementation?

I didn't want to handle every permutation and figured this would get the point across just the same. We planned to document the context option names as deprecated aliases anyway.

Note that crl_file is also not supported by secure transport, nor libressl's libtls

Thanks for noting LibreSSL's lack of support.

Is there a reason that mongoc-stream-libressl.c has a commented-out function call instead of MONGOC_ERROR() calls as found in Secure Transport's implementation?

In your opinion, should we simply skip this checking and allow libmongoc to log its error at runtime. This would result in an exception being thrown from whatever method was responsible for initializing the stream (e.g. executeCommand() for a ping in most of our SSL tests).

I'll note that our libmongoc log handler, php_phongo_log(), is only used when mongodb.debug is enabled. That function has logic to throw exceptions for ERROR and CRITICAL level log messages and log everything else to the debug file descriptor. Perhaps we should have another log handler to use when mongodb.debug is disabled so we can still provide exceptions for ERROR and CRITICAL messages while ignoring lesser logs. Thoughts?

/cc @derickr

[jmikola]

Perhaps we should have another log handler to use when mongodb.debug is disabled so we can still provide exceptions for ERROR and CRITICAL messages while ignoring lesser logs.

@derickr made a good point that this would simply cause existing code to start throwing new or different exceptions out of the blue. Grepping the libmongoc code, SSL option parsing appears to be one of the few places where an ERROR is logged (to report an option being ignored) and the code continues doing its thing without immediately returning. I suppose we'll stick with checking for these during PHPC option parsing.

[jmikola]

@bjori: I've split up the exception messages for capath and ca_dir in a later commit and also added a separate exception for the unsupported crl_file option.

[bjori]

Perhaps we should have another log handler to use when mongodb.debug is disabled so we can still provide exceptions for ERROR and CRITICAL messages while ignoring lesser logs. Thoughts?

I think there might be a ticket for that already.

MONGOC_WARNING and MONGOC_ERROR should always result in exceptions.

@derickr made a good point that this would simply cause existing code to start throwing new or different exceptions out of the blue.

You do want to do that. You can't silently ignore ssl configuration like this.
If the application is relying on providing its custom capath (highly unlikely) then it should error as it doesn't work, and they'll have a hellofaconfusing time debuging why not, unless you throw exception explaining it.

[jmikola]

You do want to do that. You can't silently ignore ssl configuration like this.

Isn't that what libmongoc is doing by merely logging an error and allowing SSL initialization to continue? We generally do check for errors after calling libmongoc functions and throw accordingly, so also throwing on ERROR and CRITICAL logs will likely just result in two exceptions being thrown in most cases (with the first getting stuffed under the most recently thrown).

Anywho, we are not catching the unsupported options at the time of URI and option processing, so I think we're OK here.

[bjori]

Isn't that what libmongoc is doing by merely logging an error and allowing SSL initialization to continue?

Looks like that was indeed what we did in couple of places. I've filed a ticket and CR for it:
https://jira.mongodb.org/browse/CDRIVER-1532

[derickr]

LGTM