Feature #20057
closedChange behaviour of rb_register_postponed_job for Ruby 3.3
Description
This ticket is to discuss some changes to rb_register_postponed_job
that @ko1 (Koichi Sasada) and myself propose to make for Ruby 3.3. The motivation for this work is to fix a bug in the current implementation, which can cause the registered functions to be called with the wrong data
argument (https://bugs.ruby-lang.org/issues/19991).
There's a long discussion on the associated PR (https://github.com/ruby/ruby/pull/8949) but in the end we came to the conclusion that the best way to fix this bug involved actually changing the current semantics of rb_register_postponed_job
. I'm opening this issue to get feedback on this approach and to see if anybody knows of a reason why we should not release this for Ruby 3.3.
Current behaviour in Ruby 3.2¶
Currently, Ruby has two functions for interacting with postponed jobs. These jobs can be enqueued from anywhere (including signal handlers), and will be executed next time Ruby checks for RUBY_VM_CHECK_INTS()
.
-
rb_postponed_job_register(func, data)
: Schedulesfunc(data)
to be executed the next timeRUBY_VM_CHECK_INTS
is checked. -
rb_postponed_job_register_once(func, data)
: Works likerb_postponed_job_register
, except iffunc
is already scheduled to be executed (either with thisdata
or with differentdata
), in which case it does nothing.
The postponed jobs are stored in a fixed sized array (of length 1024), so it's possible that enqueuing them could fail if the buffer is full. In this case, they signal this by returning 0
(otherwise, they return 1
for successful enqueue or 2
because rb_postponed_job_register_once
did nothing because func
was already in the queue).
Unfortunately, as I mentioned before, the implementation of these functions are subject to a race condition because func
and data
are not written into the postponed job buffer together atomically (they are two separate variables and CPUs tend not to have double-word atomic instructions). Again, see https://bugs.ruby-lang.org/issues/19991 for the full details.
What we have done¶
Whilst working on this issue, we had a look at all of the in-the-wild usages of these APIs on rubygems. The only real usage of these APIs is for profiling tools, and the following was true for essentially all of them:
- Each gem only is registering a single callback function,
- Almost all of the usages either make no use of the
data
argument at all, or pass some kind of never-changing global context into it. - There are only a very small handful of gems using these APIs at all
Thus, we concluded that the current behaviour of allowing scheduling and execution of arbitrary (func, data)
pairs is actually not really needed. Instead, we could offer a more limited API which would meet the needs of all current users, whilst making it easy to avoid the race conditions in the current implementation.
The new API is as follows:
-
rb_postponed_job_preregister(func, data)
: This function registersfunc
/data
into a small, fixed-size table, and return a handle to this registration. Subsequent calls to this function with the samefunc
will return the same handle, and overwrite thedata
with new data if it is different. The size of the table is 32 entries on most systems, which is still enough to use literally every gem on rubygems that actually uses these APIs at the same time. The intention is that libraries would call this function in their initialization routines, storing the handle for later. -
rb_postponed_job_trigger(handle)
: This function takes the handle fromrb_postponed_job_preregister
and schedules it for execution the next timeRUBY_VM_CHECK_INTS
is called. If the handle is already scheduled, this will not cause it to be scheduled twice; eachfunc
can only be called a maximum of one time for each call toRUBY_VM_CHECK_INTS
, essentially.
All of the usages of the old rb_postponed_job_register{,_once}
functions in the Ruby tree have been replaced by calls to the above two functions, and these two old functions have been marked with the deprecated attribute. They have also been re-implemented in terms of the new functions; both rb_postponed_job_register
and rb_postponed_job_register_once
are now both equivalent to rb_postponed_job_trigger(rb_postponed_job_prereigster(func, data))
. This means that:
-
rb_postponed_job_register
now works likerb_postponed_job_register_once
i.e.func
can only be executed one time perRUBY_VM_CHECK_INTS
, no matter how many times it is registered - They are also called with the last
data
to be registered, not the first (which is howrb_postponed_job_register_once
previously worked)
I verified that stackprof still builds & works correctly with the new implementation of rb_postponed_job_register
.
What else we tried¶
I tried a couple of things to keep the current semantics of rb_postponed_job_register{,_once}
intact, without introducing new APIs.
- First, I tried protecting postponed job buffer by masking signals around the critical section & using a POSIX semaphore instead of a pthread mutex: https://github.com/ruby/ruby/pull/8856. However, there was a concern that this would be too slow (since
RUBY_VM_CHECK_INTS
is called very often, and both the semaphore and the signal mask require calling into the kernel). - Then, I implemented a lock-free ringbuffer to store the postponed job queue: https://github.com/ruby/ruby/compare/master...KJTsanaktsidis:ruby:old_circular_ringbuffer. However, the concern with this implementation was that it was too complex.
Ruby 3.3¶
As of right now, we have merged these changes (from https://github.com/ruby/ruby/pull/8949), and @ko1 (Koichi Sasada) plans for them to go out in 3.3-rc1. The point of opening this issue is to ask: does anybody foresee any problem with our approach?
Updated by kjtsanaktsidis (KJ Tsanaktsidis) 11 months ago
- Description updated (diff)
Sorry, I linked the wrong PR - the correct one is https://github.com/ruby/ruby/pull/8949. Updated the description.
Updated by Eregon (Benoit Daloze) 11 months ago
Is it necessary to change the data for an existing callback?
Alternatives I can think of:
- Return error code if the callback is already registered.
- Allow registering multiple times the same callback.
From reading the description of this issue it's not clear to me why the 2 functions are safer or why we would need to deprecate the older API, given the older API just calls the new one.
Updated by kjtsanaktsidis (KJ Tsanaktsidis) 11 months ago
Is it necessary to change the data for an existing callback?
There was, afaict, at least one gem (gctools
) which did actually want to call the same function with a different callback at various points in the program. A previous version of the patchset I proposed did actually "allow registering the callback multiple times" as you suggested, but repeated registration of the same callback with different data would fill the postponed job table, and necessitate some kind of cleanup, which brings back the original race condition. Thus, we landed on this solution instead.
From reading the description of this issue it's not clear to me why the 2 functions are safer or why we would need to deprecate the older API, given the older API just calls the new one.
From a strictly functional perspective, we don't need the newer API, we could just only make the change to the current APIs. But, the old APIs kind of imply from their signature that you could call them with arbitrary func
and data
as much as you like, whilst the new APIs encourage the pattern of registering a small number of func
's during program initialization (where an error could actually conceivably be handled, too). I don't think there's a compelling argument to actually delete the older APIs though, I more marked them as deprecated to nudge people towards using the correct pattern.
Updated by Eregon (Benoit Daloze) 11 months ago
There was, afaict, at least one gem (
gctools
) which did actually want to call the same function with a different callback at various points in the program. A previous version of the patchset I proposed did actually "allow registering the callback multiple times" as you suggested, but repeated registration of the same callback with different data would fill the postponed job table, and necessitate some kind of cleanup, which brings back the original race condition. Thus, we landed on this solution instead.
Could changing data be something handled by the caller, e.g., instead of passing data
, pass &data
and let the caller change it when it wants?
But I guess that would be thread unsafe so probably not a good solution.
From a strictly functional perspective, we don't need the newer API, we could just only make the change to the current APIs. But, the old APIs kind of imply from their signature that you could call them with arbitrary
func
anddata
as much as you like, whilst the new APIs encourage the pattern of registering a small number offunc
's during program initialization (where an error could actually conceivably be handled, too). I don't think there's a compelling argument to actually delete the older APIs though, I more marked them as deprecated to nudge people towards using the correct pattern.
Thanks for the clarification.
I think the proposed change is good.
Updated by kjtsanaktsidis (KJ Tsanaktsidis) 11 months ago
- Status changed from Open to Closed
Applied in changeset git|a2994c300b9620b0b226c8373f15627eead65d43.
Add changelog entry for [Feature #20057]
Updated by kjtsanaktsidis (KJ Tsanaktsidis) 11 months ago
- Status changed from Closed to Open
Sorry, didn't mean to close this.
Updated by ko1 (Koichi Sasada) 11 months ago
rb_postponed_job_preregister()
, rb_postponed_job_register/_once()
run O(n) where n is registered jobs.
rb_postponed_job_trigger()
is O(1).
We believe n
is enough small though...
Updated by kjtsanaktsidis (KJ Tsanaktsidis) 10 months ago
- Status changed from Open to Closed
This was merged for Ruby 3.3 - f8effa209adb3ce050c100ffaffe6f3cc1508185