PHPC-987: Causally consistent reads

[jmikola]

https://jira.mongodb.org/browse/PHPC-987

[derickr]

Fails to compile on PHP 5 with zts: https://travis-ci.org/mongodb/mongo-php-driver/jobs/318906532#L1550

On PHP 7 there is a test failure: https://travis-ci.org/mongodb/mongo-php-driver/jobs/318906525#L1933

Please fix the commit message, there is nothing casual about this (it's causally).

[derickr]

I'm uncomfortable with calling methods through the PHP/Zend layer here. Is there no other way to obtain the timestamp?

[jmikola]

I can certainly add logic to check for a Timestamp object and access these properties directly without invoking methods, and will do so.

That said, there is no other solution if we're dealing with a non-Timestamp TimestampInterface.

In my testing, if the getTimestamp() throws, getIncrement() is never even called. This may be the result of logic inside zend_call_method that no-ops if a previous call failed or an exception has already been thrown. I still found that a bit odd, as here I am invoking zend_call_method twice without logic in-between. If you're more comfortable with it, I can also split up those calls and check for errors between them, instead of a combined check for undefined ztimestamp and zincrement down below.

[derickr]

As this goes through zend_execute, I expected that the throw would abort further calls. I think it's better to check for EG(exception) in between. But you're right on the non-Timestamp TimestampInterface. I now believe you shouldn't short-cut it as an exception for "Timestamp" too.

[jmikola]

The IS_UNDEF check was inspired by what I saw in zend_interfaces.c:

• https://github.com/php/php-src/blob/php-7.2.0/Zend/zend_interfaces.c#L160
• https://github.com/php/php-src/blob/php-7.2.0/Zend/zend_interfaces.c#L191
• https://github.com/php/php-src/blob/php-7.2.0/Zend/zend_interfaces.c#L388

I actually think the best approach is checking for (Z_TYPE(retval) == IS_UNDEF || EG(exception)), as is done in the last example (zend_user_serialize).

I personally don't see any harm in optimizing for the common use case (Timestamp instance), and simply forgot to make that change before I last updated this PR. I'll leave it out for now given your feedback, but we can always add it in later if you change your mind.

[jmikola]

Fails to compile on PHP 5 with zts. On PHP 7 there is a test failure.

I'll look into the test failures and revise. This was admittedly pushed up for review as I was headed out the door last night (wanted you to see something before this morning), so I can't say I'm surprised.

[derickr]

I'm sorta lgtm on this, I wouldn't want to hold up the comment about calling methods through the Zend layer for getTimestamp/getIncrement. But I do think it's not a nice way of doing it. But probably better than a short-cut, and I've no idea right now how to do this better.

[derickr]

As this goes through zend_execute, I expected that the throw would abort further calls. I think it's better to check for EG(exception) in between. But you're right on the non-Timestamp TimestampInterface. I now believe you shouldn't short-cut it as an exception for "Timestamp" too.