Reorders put method upload selection to use @transfer_manager first

[hms]

Reorders put method to use @transfer_manager if defined and then fallback to existing (but soon to be deprecated code)

I did my best to maintain the code formatting, especially in the tests. But it was rather challenging given my Rubymine configuration had very, very ideas about formatting than the project does. I did my best to put things back by hand, but there were several area's where the changes were by spacing that did not show visually in Rubymine.

[hms]

I'll update the code to not use it tomorrow. Didn't even think of these older Rubies. Sorry.

[hms]

I need some feedback if the Shrine team wants me to attempt fix the Ruby 2.5 and 2.6 failures. They look like a dependent library is behaving differently.

[hms]

@adam12

This is not to pester, just to get feedback so I know what the current support team wants and if I have more work on this PR.

@hms

[adam12]

@hms I see where you're going with this, but we'd have to cut down on the formatting noise first. I was going to take a stab at it, but if you wanted to continue with it, that's fine by me.

But we won't be able to merge with all the formatting changes so that would be a first step.

[hms]

@adam12

Unfortunately fair. But to be honest, it would be helpful to have at least something to reference so I can configure my editor to honor this projects defaults.

I guess I'll reimplement the PR using VIM to avoid unexpected reformatting.

[janko]

@hms Couldn't you temporarily disable format on write in your editor? Maybe add it to project-specific config?

[hms]

@janko

I switch over to ZED for Shrine and it's much less opinionated about formatting. My latest push is clean and only touches what I've changed to make the transaction_manager the default when it exists and your original code the fallback.

hms

[janko]

Thanks for cleaning it up! I would personally prefer to keep everything in the #put method, because each conditional branch is small enough. But we can also update that after merging.

Unfortunately, in recent ruby versions the S3 tests are segfaulting. AFAIR that's a regression in Ruby where Down::ChunkedIO usage in #open probably stretches ruby's abilities, most likely it's fiber-related, but I haven't been able to reproduce it yet so that I can report it. So, please ignore those.

[hms]

@janko

I'm sorry for the kerfuffle around the formatting. I didn't even think about Rubymine and its "need" to "seamlessly change things".

Normally I would not have split #put into 2 methods. My rational is that at some point in the future, the OG #put code actually has to disappear as those methods it is based on will disappear. That may be far enough into the future that you don't want to worry about that just yet and/or you'll cool with people locking the S3 Gem to a version that retains the deprecated APIs.

Unfortunately, in recent ruby versions the S3 tests are segfaulting. AFAIR that's a regression in Ruby where Down::ChunkedIO usage in #open probably stretches ruby's abilities, most likely it's fiber-related, but I haven't been able to reproduce it yet so that I can report it. So, please ignore those.

If the download functionality was also migrated to the transfer_manager, would Shrine still need the Down Gem?

Also, just before giving up on tracing a bad bit of Shrine related memory bloat (possibly leak), it was starting to look like Down (only when using temp files) was the root cause. When I get a few minutes of free time, I'll try to reproduce those tests using the transfer_manager. Might be another reason to accelerate the full transition to the new API.

hms

[jrochkind]

If the download functionality was also migrated to the transfer_manager, would Shrine still need the Down Gem?

Yes, among other reasons because Shrine uses more than just S3 storage adapters.

But I too have been suspecting some memory issues in Down, although they may be just kind of unavoidable in ruby is also what I was maybe arriving at (the memory issues may not be particular to Down, but endemic to any attempt to copy large files in ruby). But I'm very curious as to what you may find, although it's not about this issue/PR, maybe open up a Discussion?

[hms]

@jrochkind Thanks for the gentle reminder that sometimes the problem is bigger than the one impacting me.... I really wasn't thinking outside of S3 at all.

When I find the time to dig into Down again, I'll open up a ticket over there and reference it here.

[hms]

@janko @adam12

Are you folks waiting on more changes from me to move this PR forward?

hms

[adam12]

Are you folks waiting on more changes from me to move this PR forward?

Just lacking free time. I'll try to look this weekend if it's not reviewed beforehand.

[adam12]

The test failures are due to a change in aws-sdk-s3 that is not visible except across Ruby versions.

On Ruby 2.5 & 2.6, you get aws-sdk-s3 1.188.0 but once you move to Ruby >= 2.7 you get aws-sdk-s3 1.202.0.

On 1.202.0 SDK, the operations are: [:create_multipart_upload, :upload_part, :complete_multipart_upload]

But on 1.188.0 SDK, the operations are: [:put_object].

I hate to suggest it, but I think we should add in guards for the tests that are failing/had to be adjusted. We'll guard >= 2.7.0 and < 2.7.0 perhaps.

Example:

it "does whatever" do
  # New tests
end if RUBY_VERSION >= "2.7.0" # Because aws-sdk-s3 order of operations changed on newer Rubies

it "does whatever" do
  # Old tests
end if RUBY_VERSION < "2.7.0" # Because aws-sdk-s3 order of operations changed on newer Rubies

Only for the failing tests.

[hms]

@adam12 Was that last comment addressed to me or Janko for approval?

I'm happy to make the changes if that's what's holding things up.

hms

[adam12]

Sorry, I should have been more clear.

I think the best path forward is versioning the tests by Ruby version. I don't see a better way, and we'll get the changes unblocked.

So if you make those changes, we can get this PR merged.

[janko]

I appreciate the persistence, I'll try to get a new version released tomorrow 👍🏻

[hms]

@janko Thank you for Shrine and keeping it alive despite ActiveStorage.