Fix some FFI functions incorrectly being unsafe.
#19
Conversation
src/Database/PostgreSQL/LibPQ.hsc
Outdated
| @@ -2373,6 +2373,19 @@ loUnlink connection oid | |||
| negError =<< c_lo_unlink c oid | |||
|
|
|||
|
|
|||
| -- Reminder on `unsafe` FFI: | |||
| -- | |||
| -- - `unsafe` calls MUST NOT do any IO! | |||
There was a problem hiding this comment.
Reference to this claim?
Do you rather mean "must not block"?
There was a problem hiding this comment.
That's right, I had "database IO" in mind. Will update to "blocking".
Those functions can call back or or do blocking IO, which is not allowed for `unsafe` calls. This fixes a case I encountered on our CI machine where `unsafe` on `PQisBusy()` resulted in GHC RTS hangs when a postgresql notice processor was set to call back into Haskell.
nh2
force-pushed
the
fix-reentrant-unsafe-ffi
branch
from
a119b79
to
a16375f
Compare
There was a problem hiding this comment.
This is unfortunate. If notice handler is not set (or is non-Haskell-calling function), most of these could remain unsafe, OTOH I do see a value of setting a handler too.
I also have uneasy feeling, as any of now-still-unsafe functions may start to notify in the future. Whether they call notifier is not in documentation, and would be hard to test.
How bad would be to mark all PGConn/PGResult related functions as safe?
| @@ -2636,33 +2694,42 @@ foreign import ccall "libpq-fe.h PQunescapeBytea" | |||
| -> Ptr CSize | |||
| -> IO (Ptr Word8) -- Actually (IO (Ptr CUChar)) | |||
|
|
|||
| foreign import ccall unsafe "libpq-fe.h PQescapeIdentifier" | |||
| -- Must not be `unsafe` because `PQescapeIdentifier` may call `libpq_gettext()` that may do IO. | |||
There was a problem hiding this comment.
Does it actually block? Aren't translations loaded once into memory?
There was a problem hiding this comment.
I think they are loaded on demand:
Here, bindtextdomain() sets the directory from which translation files are read, and dgettext() returns the translation string, so in between disk IO must be happening in practice.
Postgres puts an if (!already_bound) around it, but that is quite the implementation detail; to make PQescapeIdentifier() unsafe, we'd have to find some guarantee that it is imposible to call this function without some other safe function triggering the gettext code path first. This would be quite fragile.
Yes, implementation change is a risk. Even more likely than notifying that they may notify, they may call I guess that Personally I'd really like to have a flag to the GHC RTS that (with some overhead costs) tracks whether any
On https://gitlab.haskell.org/ghc/ghc/-/issues/13296#note_132110 Simon Marlow estimates the So the answer would depend on whether there are use cases that call one of these functions very quickly in a loop, e.g. to parse a multi-GB query by parts, or something like that. I have not yet investigated how likely that is to occur. In any case, I propose that we first merge this fix to fix certain hangs, and then a separate investigation can be launched to see whether a general safer default would be sensible. |
|
For some googleability, and if others have similar problems, this is how I found the cause of the hang:
|
|
i'd prefer to default to all So I'd rather error-out to the |
See #19 libpq can callback into Haskell, or do IO, so it's safer to just not use unsafe. The overhead should be minimal.
|
I made all functions |
|
Very good! |
|
I started an initiative for GHC to facilitate finding long-running GHC #25333: Add RTS stats and alerts for long-running unsafe foreign function (FFI) calls |
Those functions can be reentrant or do IO.
This fixes a case I encountered on our CI machine where
unsafeonPQisBusy()resulted in GHC RTS hangs when a ostgresql notice processor was set to call back into Haskell.