Open jjb opened 9 years ago
@jjb When using delayed job it is possible to further delay the running of a job by using something like run_at: -> { 5.minutes.from_now }
. Perhaps you can use this to rule out that the job is processed before save!
is called on User
? But as you already mentioned this should not be the case...
@baschtl Good idea. I haven't gone that route of experimentation yet because I can't reliably reproduce the problem. And also I want to know the cause of the problem before doing a workaround, just in case it's something more serious and/or that affects other things.
I figured it out!
My code uses devise's send_confirmation_token
in send_confirmation_token
, there is this code: https://github.com/plataformatec/devise/blob/master/lib/devise/models/confirmable.rb#L97
@raw_confirmation_token
is always nil, so a new one is generated, here: https://github.com/plataformatec/devise/blob/master/lib/devise/models/confirmable.rb#L227
Where, as you can see, the user object is persisted (save(validate: false)
).
So now, changed?
will be false
. Meanwhile, back in send_confirmation_instructions
: https://github.com/plataformatec/devise/blob/master/lib/devise/models/confirmable.rb#L101, send_devise_notification
is invoked. And as we know, it's devise-async's monkeypatched send_devise_notification
, which depends on checking changed?
to decide if it should enqueue or send right away: https://github.com/mhfs/devise-async/blob/master/lib/devise/async/model.rb#L38
So, that's the problem.
My solution was to add generate_confirmation_token!
to my code before incrementing my counter, like so:
class User
def remind
generate_confirmation_token! # changes model, then saves, so we're back to changed? == false
self.email_confirmation_reminder_count += 1 # changes model, leaving changed? == true
send_confirmation_instructions
save!
end
end
I haven't thought about what a more generalized solution for devise-async would be, but it seems like there could be one.
Thoughts?
Did I get this right:
changed? == true
).send_confirmation_instructions
which generates a new confirmation_token
and saves the record (changed? == false
).changed?
which is in this case always false
. Hence, this job is directly forwarded to the corresponding job backend.What I don't get is:
When in step (3) the record is not changed?
anymore your incrementation of the reminder count should already have been persisted, shouldn't it? So, why is the reminder count not as expected in the mailer template?
Another thing: the job backend only gets the id
of the record. Hence, it fetches the record again when the job is being processed. Hence, the incrementation of the reminder count should be present in the newly fetched record.
Is this then a concurrency issue (i.e., the database transaction saving the record has not been finished when the mail is sent)? Is this the root cause?
Did I get this right: ...
Yes, but note that your step (3) is actually within your step (2) -- send_confirmation_instructions
does the changing, saving, and sending.
When in step (3) the record is not
changed?
anymore your incrementation of the reminder count should already have been persisted, shouldn't it?
No, because I haven't saved it yet. In my method, the job is queued up in send_confirmation_instructions
, and then the record is persisted in the next line, save!
. Sometimes, the job runs before save!
happens. And that's when the problem happens.
Another thing: the job backend only gets the
id
of the record. Hence, it fetches the record again when the job is being processed. Hence, the incrementation of the reminder count should be present in the newly fetched record.
I think this is the same as your previous point? If not let me know :)
@jjb Ah, I see my mistake, now. :-)
Hmm, at least you could do
class User
def remind
increment!(:email_confirmation_reminder_count)
send_confirmation_instructions
end
end
Or is there a reason for first generating the confirmation token and then incrementing the count on the instance in your solution proposal?
I'll put my annotated solution here again, and then also the buggy version:
class User
def remind
# we begin with an unchanged model (changed? == false)
# changes the model (changed? == true)
# saves (changed? = false)
generate_confirmation_token!
# changes model (changed? == true)
self.email_confirmation_reminder_count += 1
# does NOT invoke generate_confirmation_token!
# sends the email, which devise async queues
send_confirmation_instructions
save!
end
end
class User
def remind
# we begin with an unchanged model (changed? == false)
# changes model (changed? == true)
self.email_confirmation_reminder_count += 1
# invokes generate_confirmation_token!
# changes the model (changed? == true)
# saves (changed? = false)
# sends the email, which devise async does NOT queue
send_confirmation_instructions
save!
end
end
@baschtl Okay, after I wrote all that, I realized I didn't understand your suggestion :-D
You are saying why not just make sure the incrementation is persisted to the database before queueing up the email? Yes you are right that is another option. I don't like it for a few reasons.
First of all I generally try to avoid increment!, update_column, etc., because they don't go through the regular ActiveRecord chain of things (validators, filters). So, I have to switch my mental model and assumptions.
Second, with how I do my code now I don't have to think about behavior if any part of the code fails and a database transaction has to be rolled back. Now, I think with your solution, it will be okay, but... I didn't want to have to think about that and figure it out :)
For both reasons above, I think your solution is "more brittle" if/when other code is introduced down the line.
Another reason: I will feel like I'm "fighting" devise and devise_async by trying to hack around their problems and assumptions instead of creating the environment they want.
Now, one might say that my solution is the more "fighting" version because generate_confirmation_token! usually happens within devise and here I am unnaturally invoking it outside of a devise method. This is perhaps a good argument :) but for some reason mine feels more natural.
Also, perhaps the more important overall point: devise_async needs to be fixed so that its queueing system doesn't break when a devise method persists an object. (right?)
@jjb Well, no need for excuses. It's your decision. ;-)
In the end, I don't know what exactly happens with this gem. As mentioned in https://github.com/mhfs/devise-async/issues/64 there might be the possibility to use Active Job. This would make devise-async superfluous. You might want to consider this as another option.
Gotcha -- yes I saw that rails 4.2 seems to have devise-async-like features.
@baschtl just curious about choosing between devise-async and Active Job, and couldn't get enough clarity from the various comments about it.
Please correct me if I'm wrong, but wouldn't active job simply queue the emails to be background processed, but won't actually solve those race conditions between the job being queued and the model not yet being committed to the database?
@gingerlime Hey, I do not have enough experience with Active Job and this particular case to tell the truth. But, yes, this issue could also occur when using Active Job.
However, you should consider using Active Job if it fits your use case as it might be more future proof.
I might have the time over the next days to take a look at this with a minimal implementation.
Thanks @baschtl, no experience with it either, although it kinda makes sense to use it when we upgrade to rails 4.2 ...
Was just wondering about the actual promise of Active Job in this particular case. I can't imagine it would, by itself, resolve this potential race condition between the job being executed and the user creation transaction committed though (devise hooks the sending after save, rathan than after commit). That's why I thought I should ask, in case someone does have any experience with it.
Here's a situation I am facing. It's very specific to my app, but I'm pretty sure has a root cause in devise-async.
I sometimes remind users who haven't confirmed their email addresses to do so. It goes something like this (some things renamed/simplified):
then in the email template...
Problem: Sometimes when sending the confirmation instructions in this way,
@user.email_confirmation_reminder_count
is0
, even though it is incremented inUser#remind
. Inspecting the object at the console after the email has been sent, the incrementing was indeed successful as expected. So, for some reason theUser
object that was accessed within the template has the value set to0
, but persisting the incremented value withinUser#remind
was successful.And as a reminder, this problem only happens sometimes.
A Pretty Good Theory
I've actually seen a similar phenomenon before, and I thought it might be the case here too. If it is the case here, then this is what happens:
User.email_confirmation_reminder_count
is incremented on the in-memory objectsend_confirmation_instructions
creates the background job. The background job is started and finishes before step 3...User
object, withemail_confirmation_reminder_count
incremented, is persisted to the DB.But That Theory Doesn't Hold With devise-async
As you may know, devise-async specifically goes to great lengths to avoid such a situation: https://github.com/mhfs/devise-async/blob/master/lib/devise/async/model.rb
Beginnings of Other Theories
email_confirmation_reminder_count
, theUser
object is not consideredchanged?
by devise-async https://github.com/mhfs/devise-async/blob/master/lib/devise/async/model.rb#L38 - but this seems basically impossibleUser
object is reloaded... somewhere... in devise, devise-async, or elsewhereHelp Me
If anyone has any ideas or suggestions for what to investigate, it would be much appreciated!
Hopefully when I solve this I can contribute some documentation that will help others!