Skip to content

[WAIT] PHPC-1274: Reset libmongoc client after forking #941

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[WAIT] PHPC-1274: Reset libmongoc client after forking #941

wants to merge 1 commit into from

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Nov 29, 2018 •
edited
Loading

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

This is not suitable for merging. Just opening so we have a POC to look at.

@derickr
Copy link
Contributor

derickr commented Nov 30, 2018

This is suitable for merging.

s/suitable/not suitable, I suppose?

@jmikola
Copy link
Member Author

jmikola commented Nov 30, 2018

s/suitable/not suitable, I suppose?

Good catch :)

@jmikola jmikola changed the title [POC] PHPC-1274: Reset libmongoc client after forking [WIP] PHPC-1274: Reset libmongoc client after forking Apr 25, 2019
@jmikola jmikola changed the title [WIP] PHPC-1274: Reset libmongoc client after forking PHPC-1274: Reset libmongoc client after forking Apr 30, 2019
@jmikola
Copy link
Member Author

jmikola commented Apr 30, 2019

Note: the current version of this PR scatters pid checks throughout the driver. Although calls to getpid() may be cached for some versions of glibc, I think the overhead might still be a concern.

An alternative approach could entail exposing the API for mongoc_client_reset() behind our own method on the Manager class, which does its own pid checking, and instruct users to manually call that after forking.

@jmikola jmikola force-pushed the phpc-1274-poc branch 2 times, most recently from 3a6f01e to 7550735 Compare May 2, 2019 16:19
sgolemon
sgolemon reviewed May 3, 2019
View reviewed changes
Copy link
Contributor

@sgolemon sgolemon left a comment

Choose a reason for hiding this comment

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

lgtm overall, just a naming nit.

php_phongo.h Outdated
@@ -212,6 +212,13 @@ zend_bool phongo_writeconcernerror_init(zval* return_value, bson_t* bson TSRMLS_
#define PHONGO_ZVAL_CLASS_OR_TYPE_NAME(zv) (Z_TYPE(zv) == IS_OBJECT ? ZSTR_VAL(Z_OBJCE(zv)->name) : zend_get_type_by_const(Z_TYPE(zv)))
#define PHONGO_ZVAL_CLASS_OR_TYPE_NAME_P(zvp) PHONGO_ZVAL_CLASS_OR_TYPE_NAME(*(zvp))

#define PHONGO_CHECK_CLIENT_PID(client, created_by_pid) \
Copy link
Contributor

Choose a reason for hiding this comment

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

This name could be more descriptive of what it does.
Perhaps: PHONGO_CLIENT_RESET_IF_CHIlD_PID(...)

@jmikola
PHPC-1274: Reset mongoc_client_t for child processes
34b9d4d 
Check pid before operations that could interfere with resources owned by the parent process. This includes freeing Cursor and Session objects, executing operations on the Manager or Server, or iterating a Cursor.
@jmikola
Copy link
Member Author

jmikola commented May 7, 2019

Standalone SSL failure is pending CDRIVER-3116.

kvwalker
kvwalker approved these changes May 9, 2019
View reviewed changes
Copy link
Contributor

@kvwalker kvwalker left a comment

Choose a reason for hiding this comment

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

looks good to me

@jmikola jmikola changed the title PHPC-1274: Reset libmongoc client after forking [WAIT] PHPC-1274: Reset libmongoc client after forking Aug 30, 2019
@alcaeus
Copy link
Member

alcaeus commented Oct 2, 2019

Closing after discussion summarised in this comment.

@alcaeus alcaeus closed this Oct 2, 2019
@jmikola jmikola deleted the phpc-1274-poc branch November 12, 2019 22:31
@jmikola jmikola mentioned this pull request Nov 12, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgolemon sgolemon sgolemon approved these changes

@kvwalker kvwalker kvwalker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jmikola @derickr @alcaeus @sgolemon @kvwalker