OperationCode / operationcode_backend

This is the backend repo for the Operation Code website
https://operationcode.org
MIT License
62 stars 110 forks source link

Fix send grid job #402

Closed nellshamrell closed 5 years ago

nellshamrell commented 6 years ago

This fixes an error we were seeing in Sentry:

ActiveJob::DeserializationErrorSidekiq
errorError while trying to deserialize arguments: Couldn't find User with 'id'=5838

Turns out there is a common race condition when running a Sidekiq job right after a create or save - see this extremely helpful article for more details.

I think that the current tests are sufficient to cover this behavior, but am willing to consider adding further testing if others think it is necessary.

apex-omontgomery commented 6 years ago

So the summary from this is that testing race conditions in the callback process for ActiveRecord is hard due to the outermost call being the test call, since each test case is wrapped in a database transaction.

Since we are on rails 5.0.2 we don't need the gem that is specified. And any test cases should now work.

robbkidd commented 6 years ago

@wimo7083 Yes to both.

We're almost there. Per the Sidekiq FAQ about this error, we should also add a constraint to the registration of this callback to prevent welcoming users whenever their User record is modified.

after_commit :welcome_user, on: :create

Possible Future ToDo:

A follow-on improvement to consider is to pass only self.id to AddUserToSendGridJob.perform_later() and update the job to do user = User.find_by(id: user_id) first thing in the perform() method. This prevents serializing the entire User record into the job data to be stored in Redis. Sidekiq's documentation on working with ActiveJob has words about this, but I've found life has been easier for debugging jobs by having the hand-off of data to a job to be as small as possible. Treat the perform_later() "scheduler" and the perform() "doer" as an API through which you pass only what's needed. This pattern makes more work for a job's perform() method and that's what gives you opportunities for error handling.

apex-omontgomery commented 5 years ago

Closing this due to continued work on : https://github.com/OperationCode/operationcode_backend/pull/407