Remove GVL unlocking at all functions which process data modifiable in a second thread - #723
Remove GVL unlocking at all functions which process data modifiable in a second thread#723larskanis wants to merge 3 commits into
Conversation
larskanis
changed the title
b9a2e30 to
56c0d7a
Compare
cbandy
left a comment
cbandy
left a comment
There was a problem hiding this comment.
📝 PR title says GLV instead of GVL
| function(PQsendQuery, GVL_TYPE_NONVOID, int, const char *, query) \ | ||
| function(PQsendQueryParams, GVL_TYPE_NONVOID, int, int, resultFormat) \ | ||
| function(PQsendPrepare, GVL_TYPE_NONVOID, int, const Oid *, paramTypes) \ | ||
| function(PQsendQueryPrepared, GVL_TYPE_NONVOID, int, int, resultFormat) \ | ||
| function(PQsendDescribePrepared, GVL_TYPE_NONVOID, int, const char *, stmt) \ | ||
| function(PQsendDescribePortal, GVL_TYPE_NONVOID, int, const char *, portal) \ | ||
| function(PQsendClosePrepared, GVL_TYPE_NONVOID, int, const char *, stmt) \ | ||
| function(PQsendClosePortal, GVL_TYPE_NONVOID, int, const char *, portal) \ |
There was a problem hiding this comment.
These are async, and docs describe them as returns 1 if it was able to dispatch the request, and 0 if not. Is it just a socket write? What can block during these?
There was a problem hiding this comment.
The PQsend* functions primary allocate and copy the data to be send into libpq's internal buffers. They try to send data, but that isn't enforced. Sending is repeatedly done by other functions, like PQflush or enforced by PQgetResult. So PQsend* functions don't block, unless libpq is broken.
larskanis
changed the title
|
I tend to say "no" to this PR. There's already the install option to avoid releasing GVL at all, which disables this kind of attack: gem inst pg -- --disable-gvl-unlockAs written here it's questionable if this kind of attack is necessary to be solved. And responses seem to acknowledge that. |
bc1fbba to
2bec478
Compare
…n a second thread This removes possible VM crashs when data to be sent is modified/cleared in a second thread. It works by keeping the GVL lock for libpq functions that don't immediately process all the data and don't make a copy of it. These are the `PQsend*`, `PQexec*` and some related functions. Since pg-1.3 all the blocking functions or states are avoided by using the non-blocking API of libpq. Therefore holding the GVL somewhat longer shouldn't matter that much. Having some libpq function with and without unlocked GVL, results in `rb_thread_call_with_gvl()` sometimes needed and sometimes not to process callbacks. Therefore `ruby_thread_has_gvl_p()` is used to check if it's needed on ruby<4.0. In ruby-4.0+ `rb_thread_call_with_gvl()` doesn't care about whether GVL is already locked or not, so that it can be called in both cases. Fixes #721
2bec478 to
b146f73
Compare
All changed functions have a non-blocking implementation in default mode `PG::Connection.async_api=true`. So there's no need to release GVL for them. The intention is to make the list of GVL-releasing functions more consistent. The only remaining functions are connection esteblishing functions, now. They are known to block in some cases (GSSAPI auth, LDAP lookup), even if used in non-blocking/default mode. So these functions should still release the GVL. These remaining functions which take a connection string as ruby object shouldn't be an issue, since this string is created in `parse_connect_args` immediately before the call. They are stored as local variables only, which are not relocated and can not be changed by other threads. `PG::Connection.async_api=false` is significant less usable, since it blocks other ruby threads at any waiting.
b146f73 to
9e95de7
Compare
Given that we now have a second issue #738, which is fixed by this PR, I withdraw my comment. I think it might be better to keep the GVL locked for most of the libpq functions. This is because #738 is a real-work issue and not a misuse of the library. The current state of this PR keeps behavior of the async API. It is the same as before. But it changes the sync API to disallow any concurrent threads. I think this is acceptable for a pg-1.7 release, since the sync API was never recommended but always behind the switch I'm not entirely sure, if we should remove GVL-release from all sync functions or only from the functions affected by #721 and #738. But it feels more correct to avoid a mix of some The downside of GVL locking of sync libpq functions is that 3 specs can no longer run on the sync API, but only on the async API, because they need threads. So we can no longer compare the behavior of these specs with sync and async API. |
2 participants
This removes possible VM crashs when data to be sent is modified/cleared in a second thread.
It works by keeping the GVL lock for libpq functions that don't immediately process all the data and don't make a copy of it.
These are the
PQsend*,PQexec*and some related functions.Since pg-1.3 all the blocking functions or states are avoided by using the non-blocking API of libpq.
Therefore holding the GVL somewhat longer shouldn't matter that much.
Having some libpq function with and without unlocked GVL, results in
rb_thread_call_with_gvl()sometimes needed and sometimes not to process callbacks.Therefore
ruby_thread_has_gvl_p()is used to check if it's needed on ruby<4.0.In ruby-4.0+
rb_thread_call_with_gvl()doesn't care about whether GVL is already locked or not, so that it can be called in both cases.Fixes #721