concurrent-ruby icon indicating copy to clipboard operation
concurrent-ruby copied to clipboard

Mark RubySingleThreadExecutor as a SerialExecutorService

Open meineerde opened this issue 1 year ago • 3 comments

This resolves #1069.

However, I'm still not 100% sure if the RubySingleThreadExecutor is actually serialized. From my understanding of the code, it is, but there may be edge-cases I'm missing (such as when the thread dies).

As such, I'd still appreciate if someone who knows the (intended) RubyThreadPoolExecutor semantics to review this.

meineerde avatar Nov 01 '24 21:11 meineerde

The old implementation doesn't look substantially different than the new one does. Both call Queue#pop and rescue from Exception, albeit the old implementation would swallow the exception whereas the new one will create a new worker thread; that doesn't change the behavior in my eyes.

Old implementation:

https://github.com/ruby-concurrency/concurrent-ruby/blob/7c2909ffbe2bcfb26410effa023b9f3f2498c65b/lib/concurrent/executor/ruby_single_thread_executor.rb#L66-L78

Current implementation:

https://github.com/ruby-concurrency/concurrent-ruby/blob/eae2851b53d988ab314030407dd5030f78db5c90/lib/concurrent-ruby/concurrent/executor/ruby_thread_pool_executor.rb#L338-L356

I'll leave this PR open a little longer to see if anyone else has thoughts, otherwise I can accept it.

bensheldon avatar Nov 01 '24 21:11 bensheldon

I should also note I didn't see any test assertions of this behavior. The only place I saw #serialized? meaningfully change test behavior is right here, though I honestly think that the test is wrong and should actually be omitting on immediacy not serialized.

https://github.com/ruby-concurrency/concurrent-ruby/blob/eae2851b53d988ab314030407dd5030f78db5c90/spec/concurrent/executor/executor_service_shared.rb#L202-L211

bensheldon avatar Nov 01 '24 21:11 bensheldon

That's what I saw too when checking if/how to adapt tests. On the other hand, the serialized? method also doesn't seem to be used that much anywhere else either.

To be honest, my original intend for my question in #1069 was to ensure that the semantics of a RubySingleThreadExecutor were in fact to serialize tasks as I want to reply on that. The correct return value of the serialized? method is then a nice bonus on top :)

Still, if this holds, it may be worthwhile to update the RubySingleThreadExecutor itself to implement serialized? as something like

def serialized?
  @max_length <= 1
end

I believe the same should be valid for the JavaThreadPoolExecutor, but I'm less familiar with the exact semantics of those. The documentation however seems to imply this.

meineerde avatar Nov 01 '24 22:11 meineerde