Sirupsen/sysvmq — pull request #3 by Sirupsen (2014-02-19 17:24:56+00:00)
Title: Use rb_thread_call_without_gvl instead of gvl2

This changes from using `rb_thread_call_without_gvl2()` to `rb_thread_call_without_gvl()`. The intent of using either of those two methods is that a blocking call to `msgrcv(2)` (when the queue is empty and `IPC_NOWAIT` hasn't been passed) or `msgsnd(2)` (when the queue is full and `IPC_NOWAIT` hasn't been passed) would block the entire VM. Originally, `rb_thread_call_without_gvl2()` was chosen, however, I ran into two different errors when running the test suite multiple times, at random:

1. The receive method would segment fault.
2. The receive method would try to call `rb_sys_fail()` but fail, because `errno` was zero.

The difference between the two methods is (vaguely) documented in the [MRI source](https://github.com/ruby/ruby/blob/2e6b97a45d077979121b29484a8831034d47ef50/thread.c#L1238-1322):

```text
* rb_thread_call_without_gvl() does:
* (1) Check interrupts.
* (2) release GVL.
* Other Ruby threads may run in parallel.
* (3) call func with data1
* (4) acquire GVL.
* Other Ruby threads can not run in parallel any more.
* (5) Check interrupts.
*
* rb_thread_call_without_gvl2() does:
* (1) Check interrupt and return if interrupted.
* (2) release GVL.
* (3) call func with data1 and a pointer to the flags.
* (4) acquire GVL.
```

The cause for symptoms 1) and 2) is that `gvl2()` would interrupt the wrapping function for `msgrcv(2)` (`sysvmq_maybe_blocking_receive()`) and "return if interrupted" as the documentation says. In that case, `arguments->retval` (the return value from the system call) would just be a garbage number (positive or negative) if interrupted at the exact right time, which could cause either code path:

1. In the case of a positive number, it'd pass the error check for `arguments->error < 0`. Depending on the value, it'd either: a) cut off the buffer, because the garbage number was less than the buffer size, b) cause a segment fault because the garbage number would be larger than the buffer size.
2. In the case of a negative number, `errno` would not be set and `rb_sys_fail()` would complain that an error was reported, but `errno` wasn't set.

Whereas `gvl()` would just check and process the signals. It's still unclear to me the exact case where you want `gvl2()`, I could understand if there's a crazy edge-case, but I'm not really doing anything crazy, so I'm going to go with `gvl()` for now. Unless anyone has any objections.

This patch changes `gvl2()` calls to `gvl()`, but picks up what was learned form this bug and initializes the error value (just in case) and safe-guards with an assertion.

Obviously, blocking calls are quite difficult to get right. In Shopify, we always use `IPC_NOWAIT` to make sure we never block. I'll add a check for the `IPC_NOWAIT` flag and in that case, not release and acquire the VM. It's slow, and very error-prone.

@csfrancis @camilo @fbogsany

\cc @burke @wvanbergen @graemej @justinplouffe