Fix httpfs deadlock if there is a s3 region redirect

[NiclasHaderer]

The deadlock happens when calling S3FileHandle::Initialize and force_download set to true.

1. We call S3FileHandle::Intialize with a FileOpener which in turn calls the HTTPFileHandle::Initialize. The FileOpener is only used to retrive the ClientContext and the HTTPState. The HTTPState contains references to all cached files for this ClientContext.
2. During HTTPFileHandle::Initialize we get a reference to this CachedFile. That CachedFile is then resolved to a CachedFileHandle which locks the CachedFile. As long as the CachedFileHandle is alive and the CachedFileHandle has not been initialized the CachedFile is locked and no new CachedFileHandle can be created. The CachedFileHandle is now stored in the HTTPFileHandle which in turn is extended by the S3FileHandle.
3. If HTTPFileHandle::Initialize fails (because we have been redirected and 301!=200) S3FileHandle::Intialize retries the initialization using HTTPFileHandle::Initialize with a different S3 region. For this retry the CachedFileHandle says alive, because it HTTPFileHandle which owns it is still alive.
4. For the second call of HTTPFileHandle::Initialize we have the problem that the CachedFileHandle is still holding the lock for CachedFile, but HTTPFileHandle::Initialize stull tries to create a new CachedFileHandle. This is not possible because the already existing CachedFileHandle still holds the lock on CachedFile.

This PR fixes this by releasing the lock the CachedFileHandle holds if we fail to read from S3. Additionally it implements the region retry mechanism for more S3 call paths.

[NiclasHaderer]

@carlopi I think the CI failure is a flake

[NiclasHaderer]

@carlopi can we merge this?

[taniabogatsch]

@NiclasHaderer - we'll get back to you here shortly!

[dentiny]

Thanks for the fix!
This PR actually contains two orthogonal changes: deadlock fixing and region retry, I'm wondering if we could split it into two PRs? The former is a bug fix, the later is an enhancement.
Bugfix part LGTM

[dentiny]

BTW, just share some my idea here:

• Your fix reuses the same cached file handle to ensure lock doesn't acquire for multiple times -- I think that does fix the problem
• But that adds an implication -- on every possible place where CachedFileHandle is used, we have to reuse the same handle on retry, otherwise we suffer deadlock, that should be clearly documented on CachedFileHandle so agents could know the correct pattern; I'm wondering if you think it better to clear buffer and release lock on failed IO (for now the only place is FullDownload), so we could retry without mind overhead?

Pseudocode

void FullDownload() {
  try {

  } catch (...) {
    cached_file_handle.ResetBuffer(); // under lock
    cached_file_handle.reset(); // release lock
    throw;
  }
}

[NiclasHaderer]

Thanks for the feedback. Implemented your requested changes.

I'd prefer to not split the PR if its OK with you. I'm currently super busy and the PR is not super huge. Is that OK with you?

[dentiny]

Overall LGTM, could you please add doc for the region redirect logic? Either in the code or in the PR description.

[dentiny]

The constant has appeared for multiple times in the code change, let's make it a constant variable and add comment.

[dentiny]

I'm wondering if we could add a WARNING level logging?

[dentiny]

Hi @NiclasHaderer , we're investigating a deadlock issue which could be related to your fix, since I didn't see this PR getting updated for two weeks, I copy part of your change in this PR to #363.

[NiclasHaderer]

I did all you asked for and added docs in the or description, so as far as I'm concerned this can be merged

[dentiny]

I did all you asked for and added docs in the or description, so as far as I'm concerned this can be merged

Thanks for the reply! So

• I left two minor followup comments here and here. Do you think that makes sense to you? I didn't see update commits later on, or did I miss anything?
• The CI seems to fail on the latest commit, could you please merge with main and retry?