Skip to content

Conversation

@mainmethod
Copy link

PR to address #94

It seems that in synchronous execution with perform_sync (an alias of perform_inline), sidekiq is attempting to set the job.bid like so https://github.com/sidekiq/sidekiq/blob/96f867cb58b7fa0a6a832af1a732a339aa0eb61f/lib/sidekiq/job.rb#L236, and sidekiq-batch's worker extension does respond_to :bid, but only as a reader.

This PR adds the minimum viable code to support execution of synchronous jobs by having them also defined as atter_writer, and an instance var.

I did make the assumption that this gem needs to rely on the current thread's execution context first, then fallback to the instance var when applicable. Happy to restructure if need be

@lionelchauvin
Copy link

lionelchauvin commented Oct 17, 2025

In the case of perform_async, your code is not thread safe.
In my opinion @bid must be used only if @opts["sync"] == true

@mainmethod
Copy link
Author

mainmethod commented Oct 20, 2025

In the case of perform_async, your code is not thread safe. In my opinion @bid must be used only if @opts["sync"] == true

@lionelchauvin i see what you mean, and i do agree. however, i don't see any way to tell if it's being called in a sync or async context within this module (e.g. @opts["sync"] == true).

given @bid would only be set in sidekiq's synchronous execution https://github.com/sidekiq/sidekiq/blob/c4c92b6be5908c9062734c594431a5f8708a3b8c/lib/sidekiq/job.rb#L236C9-L236C54, i felt this was safe enough. it should never reach the other code in asynchronous execution.

ideally it would be better if that LOC in sidekiq was

job.bid = msg["bid"] if job.respond_to?(:bid=)

which they do use elsewhere https://github.com/sidekiq/sidekiq/blob/c4c92b6be5908c9062734c594431a5f8708a3b8c/lib/sidekiq/testing.rb#L288

regardless, i'm happy to change how this is being checked if you have any ideas on how I could explicitly check for something like @opts["sync"] == true or some way of checking if the current context is sync vs async

** EDIT
I see now, and assume this is what you're referring to https://github.com/sidekiq/sidekiq/blob/c4c92b6be5908c9062734c594431a5f8708a3b8c/lib/sidekiq/job.rb#L206

i don't think the Worker module has direct access to this, but i'll see what i can do

@lionelchauvin
Copy link

lionelchauvin commented Oct 22, 2025

you are right, @opts is not available in the job.

Why not assign Thread.current[:batch] instead of use an instance variable ? If it is inline/sync there should be only one batch at the time in the current thread no ?

** EDIT
Forget what I said, it is a bad idea because it would replace a batch previously set in Thread.current.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants