Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop the dispatching of new messages when a SIGTERM signal has been received #750

Merged
merged 4 commits into from
Oct 26, 2023

Conversation

hoffi
Copy link
Contributor

@hoffi hoffi commented Oct 20, 2023

Fixes #749

@phstc phstc enabled auto-merge (squash) October 22, 2023 18:06
@phstc
Copy link
Collaborator

phstc commented Oct 22, 2023

@hoffi thanks for the submission. That's greatly appreciated.

Some specs are failing but don't seem related to your change. I don't recall those being flaky before. But it seems they are also failing here #747

Can you have a look? 🙏 👀

@hoffi
Copy link
Contributor Author

hoffi commented Oct 23, 2023

The enqueued_at timestamp in job.serialize differs slightly from the value in hash here: https://github.com/ruby-shoryuken/shoryuken/blob/main/spec/shared_examples_for_active_job.rb#L47

I have looked into the #serialize method from ActiveJob and it always returns a new timestamp: https://github.com/rails/rails/blob/7-1-stable/activejob/lib/active_job/core.rb#L122

But the correct enqueued_at timestamp is not stored in the job object, so i'm not sure where to get it. Should we use something like ActiveSupport::Testing::TimeHelpers#freeze_time to ensure the same timestamp is used?

Or just use the hash argument instead of job.serialize and generate the hash without the job_id from that?

@phstc
Copy link
Collaborator

phstc commented Oct 23, 2023

@hoffi freeze_time seems like a good idea, WDYT? It seems the hash requires more changes.

I don't know why it's only happening now 🤷

auto-merge was automatically disabled October 25, 2023 17:35

Head branch was pushed to by a user without write access

@hoffi hoffi force-pushed the stop-dispatching-on-termination branch from 82c3e6a to b1f81fa Compare October 25, 2023 17:43
@hoffi
Copy link
Contributor Author

hoffi commented Oct 25, 2023

@phstc i have added the freeze_time helper which fixed the test on my machine

@phstc
Copy link
Collaborator

phstc commented Oct 25, 2023

@phstc i have added the freeze_time helper which fixed the test on my machine

@hoffi That's awesome! Thank you

https://github.com/ruby-shoryuken/shoryuken/actions/runs/6644142158/job/18052725694?pr=750#step:4:34

undefined method `freeze_time' for #<RSpec::ExampleGroups::ActiveJobQueueAdaptersShoryukenAdapter::Enqueue::WhenFifo:0x00555b2f3f[34](https://github.com/ruby-shoryuken/shoryuken/actions/runs/6644142158/job/18052725694?pr=750#step:4:35)48>
     Shared Example Group: "active_job_adapters" called from ./spec/shoryuken/extensions/active_job_adapter_spec.rb:6
     # ./spec/shared_examples_for_active_job.rb:49:in `block (4 levels) in <top (required)>'

Unfortunately, it's failing for Rails 4. Is there any alternative we could use? Maybe https://github.com/travisjeffery/timecop?

@hoffi
Copy link
Contributor Author

hoffi commented Oct 25, 2023

Yes, i have seen now it is only available since rails 5.

However i just realized that it might be problematic to include the enqueued_at timestamp in the deduplication id?

isn‘t it the same as with #457?

@phstc
Copy link
Collaborator

phstc commented Oct 25, 2023

@hoffi hm for standard works, we use only the body

options[:message_deduplication_id] ||= Digest::SHA256.hexdigest(options[:message_body].to_s)

It's probably OK to remove enqueued_at. The uniqueness should be based on what the user sent, not on what was generated.

It makes me a bit uncomfortable to change something like that after so long (especially given that it was working before), but rationally thinking makes it feel safe.

I'm up for either change: Timecop or removing enqueued_at.

If you prefer the enqueued_at removal, can you test it locally to check if the deduplication works?

@hoffi
Copy link
Contributor Author

hoffi commented Oct 26, 2023

It makes me a bit uncomfortable to change something like that after so long (especially given that it was working before), but rationally thinking makes it feel safe.

Yes however it was changed in rails 6 that the enqueued_at timestamp is returned in the message body:

If you prefer the enqueued_at removal, can you test it locally to check if the deduplication works?

I will try to see if i can test it

@phstc
Copy link
Collaborator

phstc commented Oct 26, 2023

Yes however it was changed in rails 6 that the enqueued_at timestamp is returned in the message body

@hoffi if that's the case, let's remove it then. It feels way safer given that ^ the deduplication was added prior to Rails 6.

Thanks for thoroughly reviewing this.

@hoffi
Copy link
Contributor Author

hoffi commented Oct 26, 2023

@phstc i have removed the timestamp. Can sou take a look at it?

@phstc phstc merged commit a2e2c89 into ruby-shoryuken:main Oct 26, 2023
19 checks passed
@phstc
Copy link
Collaborator

phstc commented Oct 26, 2023

@hoffi looks great! Awesome work. Thank you

@hoffi
Copy link
Contributor Author

hoffi commented Oct 26, 2023

Nice! Thanks :)

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.

SIGTERM does not stop dispatching of new messages from the queue
2 participants