Navigation Menu

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

Manage transaction rollback #2209

Merged

Conversation

skosh
Copy link

@skosh skosh commented Jul 26, 2017

When we try to create new active record object into transaction and meet rollback:
database row will not be created but uploader has created file in folder and doesn't remove it.

I changed behavior: now we store file only after commit action. This true and for update action in transaction.

Changes in spec/orm/activerecord_spec.rb:
- Add tests which covers this flows
- replace cleaning folder from 'file_path' to 'public_path'. Cause file_path is 'spec/fixtures'
and we don't create any 'uploads' folders there. We works with 'spec/public/uploads' folder.

@skosh skosh force-pushed the manage_transaction_rollback branch 3 times, most recently from 4ad26be to b0ed530 Compare July 26, 2017 20:54
  When we try to create new active record object into transaction and meet rollback:
    database row will not be created but uploader has created file in folder and doen't remove it.

  I changed behavior: now we store file only after commit action. This true and for update action in transaction.

  Changes in spec/orm/activerecord_spec.rb:
    - add tests which covers this flows
    - replace cleaning path from 'file path' to 'public_path' folder. Cause folder 'spec/fixtures/uploads ' is not used.
@skosh skosh force-pushed the manage_transaction_rollback branch from b0ed530 to 665f225 Compare July 26, 2017 21:01
@oakypinky
Copy link

👍

1 similar comment
@sashakovryga
Copy link

👍

@mshibuya mshibuya added this to the Release v2.0.0 milestone Jul 17, 2018
@mshibuya mshibuya merged commit 5faff42 into carrierwaveuploader:master Mar 30, 2019
@mshibuya
Copy link
Member

Great work, this will be included in 2.0.0 release!

fschwahn added a commit to denkungsart/carrierwave-activejob that referenced this pull request Dec 17, 2019
In carrierwave 2.0, files are no longer stored in the transaction, meaning this workaround is no longer necessary (see carrierwaveuploader/carrierwave#2209)

This reverts commit 10a2744.
@fsateler
Copy link
Contributor

I believe this change should be reverted. I'm not saying it doesn't fix the issue it fixes, but it introduces other problems:

  • Uploader state is inconsistent between save and commit. For example, reading my_object.file_url.url will return a bogus url if I read before commit.
  • A file might not be uploaded after a successful commit. For example, if there is a network failure. This causes the transaction to not be rolled back and invalid records being recorded.
  • Even if the upload eventually completes, it can take an arbitrary amount of time, during which the record is invalid.

Thus, this change can introduce data loss and data corruption.

I believe a correct fix for the original issue is to add an after_rollback callback that (tries to) removes the destination files.

@mshibuya
Copy link
Member

@fsateler Makes sense, could you open a new issue for that? Or even better if you could work on such an implementation.

@fsateler
Copy link
Contributor

Sure, done at #2545

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.

None yet

5 participants