search

jrgifford / delayed_paperclip

Process your Paperclip attachments in the background with delayed_job or Resque.
http://www.jstorimer.com/ruby/2010/01/30/delayed-paperclip.html
MIT License
402 stars 155 forks source link

Solve issue 101 #152

Closed etagwerker closed 9 years ago

etagwerker commented 9 years ago

Hey @jrgifford!

At my company, we encountered issue #101 while trying to use delayed_paperclip with Sidekiq.

I think that the best solution would be to ignore instances that have been deleted before the background job gets to the processing part.

Please check it out and let me know if it looks good.

Thanks!

ScotterC commented 9 years ago

@etagwerker Looks solid. Thanks for the contribution and tests!

etagwerker commented 9 years ago

@ScotterC Awesome! Thanks for the quick response! :beers:

ScotterC commented 9 years ago

@etagwerker now that you know the codebase, I'd love for any other contributions you can give. Sadly I'm not as active of a maintainer as I used to be

etagwerker commented 9 years ago

@ScotterC I'll try to help, but I'm already busy with a couple other open source projects.

fabiolnm commented 9 years ago

I disagree. If a record is deleted before the job is processed, it would be natural to expect RecordNotFound error on queue side. If someone wants to prevent the error, the application should explicitly dequeue the job when record is deleted.

For instance, sometimes delayed_paperclip run into race conditions against database (the job is dequeued before database transaction commits***). If queue backend has retry support, raising RecordNotFound error will gracefuly reschedule job.

*\ I still can't figure out why delayed_paperclip has race condition problems even if job seems to be scheduled on after_commit hook.

https://github.com/jrgifford/delayed_paperclip/blob/cd635ad20bf4f8dda0bfa50440d7cd479d3ba504/lib/delayed_paperclip.rb#L69-L75

johanlunds commented 8 years ago

Agree with previous commenter. We are upgrading delayed_paperclip in our app now. I think we will have problems if the background job doesn't raise an error.

Our app writes to a db master, but will read from a db slave which can have replication lag, meaning sometimes the db row doesn't exist yet when the background processing starts. In those cases we want ActiveRecord::NotFoundError to be raised and the job to fail so it can be requeued.

We will investigate this potential issue on our end to see if we have to do anything.