Revert "Manage transaction rollback"

[fsateler]

This commit attempts to fix leftover files in failed transactions, but
in the process introduces three problems:

• Uploader state is inconsistent between save and commit. For example, reading my_object.file_url.url will return a bogus url if 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 (pointing to a non-existent-file) being recorded.
• Even if the upload eventually completes, it can take an arbitrary amount of time, during which the record is invalid.

This reverts commit 665f225 from #2209.

Fixes: #2544

[mshibuya]

Just reverting will bring the original issue back again, what we need will be:

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

[fsateler]

Just reverting will bring the original issue back again

Agreed. But given the choice between stale files and missing files, I'd prefer the first (thus the "naked" revert).

what we need will be:

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

I've been poking at this but a new codebase always takes time.

[fsateler]

Hi I added a second commit adding the after_rollback callback.

[fsateler]

Hi I added a second commit adding the after_rollback callback

Scratch that. It fails for rails < 6, I'm investigating.

[fsateler]

I added the followup changes on a separate PR, so that this can be merged and restoring the other functionality is worked apart.

[mshibuya]

I don't see any benefit in merging this separately, let's close this and discuss further in #2546.

[fsateler]

I don't see any benefit in merging this separately, let's close this and discuss further in #2546.

I disagree. #2209 introduced a new feature (yay!) but introduced a very serious bug (bummer!). I would prefer carrierwave to keep storing stale data rather than lose the data I'm interested in.

[makmic]

There are other reasons to keep the call to store! in after_save. For example, I'm extracting and storing text contents from PDF files in after :store to a separate database column.
Now if the callback is called after_commit I cannot use the same database transaction to update the text content column. I have to call save in after_commit and are likely to trigger an endless callback loop!