search

Azure / azure-storage-php

Microsoft Azure Storage Library for PHP
MIT License
217 stars 196 forks source link

Please improve retry logic #185

Open udf2457 opened 5 years ago

udf2457 commented 5 years ago

At the moment there seems to be very little (if any ?) retry logic. Retry logic is somewhat critical when dealing with cloud services, therefore I would urge you not to ignore it when coding (it should be there from the start, not an afterthought).

For example, it is not uncommon to see azure-storage-php failing with:

In CurlFactory.php line 186: cURL error 56: (see http://curl.haxx.se/libcurl/c/libcurl-errors.html)

From the cURL website:

CURLE_RECV_ERROR (56) Failure with receiving network data.

This is therefore a prime example of why correct retry logic is so critical.

If I am uploading large files, azure-storage-php failing without retrying is pretty useless.

Also ServiceException does not report the cURL example shown above ?? The cURL exception is unhandled by the azure-storage-php code.

XiaoningLiu commented 5 years ago

Thanks for feedback! We have built-in retry middlewares see https://github.com/Azure/azure-storage-php/blob/master/azure-storage-common/src/Common/Middlewares/RetryMiddleware.php and https://github.com/Azure/azure-storage-php/blob/master/azure-storage-common/src/Common/Middlewares/RetryMiddlewareFactory.php

The built-in retry logic supports retrying with customized retry count, timeout, and it should retry for server errors (5xx) and network errors (like the cURL you mentioned). The retry decider is defined here:

https://github.com/Azure/azure-storage-php/blob/9e53aa6e447e2260a29d54b96fab9fe18b665afa/azure-storage-common/src/Common/Middlewares/RetryMiddlewareFactory.php#L149

You can inherit RetryMiddlewareFactory and overload the createRetryDecider method to customize the logic.

I agree that cURL 56 is kind of network error and should be retried, and also curious about why it doesn't get retired. If it's not, that's a bug we should fix.

BenWalters commented 3 years ago

@XiaoningLiu any update on this?

XiaoningLiu commented 3 years ago

@BenWalters Can you share your error logs?

BenWalters commented 3 years ago

@XiaoningLiu 

    "class": "GuzzleHttp\\Exception\\RequestException",
    "message": "cURL error 56: OpenSSL SSL_read: SSL_ERROR_SYSCALL, errno 104 (see https://curl.haxx.se/libcurl/c/libcurl-errors.html)",
    "code": 0,
    "file": "/var/www/vendor/guzzlehttp/guzzle/src/Handler/CurlFactory.php:201",
    "trace": [
        "/var/www/vendor/guzzlehttp/guzzle/src/Handler/CurlFactory.php:155",
        "/var/www/vendor/guzzlehttp/guzzle/src/Handler/CurlFactory.php:105",
        "/var/www/vendor/guzzlehttp/guzzle/src/Handler/CurlMultiHandler.php:201",
        "/var/www/vendor/guzzlehttp/guzzle/src/Handler/CurlMultiHandler.php:130",
        "/var/www/vendor/guzzlehttp/guzzle/src/Handler/CurlMultiHandler.php:145",
        "/var/www/vendor/guzzlehttp/promises/src/Promise.php:248",
        "/var/www/vendor/guzzlehttp/promises/src/Promise.php:224",
        "/var/www/vendor/guzzlehttp/promises/src/Promise.php:269",
        "/var/www/vendor/guzzlehttp/promises/src/Promise.php:226",
        "/var/www/vendor/guzzlehttp/promises/src/Promise.php:62",
        "/var/www/vendor/microsoft/azure-storage-blob/src/Blob/BlobRestProxy.php:3018",
        "/var/www/vendor/league/flysystem-azure-blob-storage/src/AzureBlobStorageAdapter.php:237",
        "/var/www/vendor/league/flysystem-azure-blob-storage/src/AzureBlobStorageAdapter.php:152",
        "/var/www/vendor/league/flysystem/src/Filesystem.php:57",
        "/var/www/vendor/league/flysystem/src/Filesystem.php:98",
        "/var/www/vendor/laravel/framework/src/Illuminate/Filesystem/FilesystemAdapter.php:231",

That's using

XiaoningLiu commented 3 years ago

Track this in next release.

BenWalters commented 3 years ago

Thanks @XiaoningLiu, what are you proposing as a solution?

XiaoningLiu commented 3 years ago

Will add retry RequestException mentioned in default retry policy.

BenWalters commented 3 years ago

@XiaoningLiu thanks, do you have any idea of the root cause of the failure in the first instance?

XiaoningLiu commented 3 years ago

No. Looks like a network or SSL error which is reported from curl underlayer.

BenWalters commented 3 years ago

@XiaoningLiu sorry for the questions! I've been working with MS Azure Support in an attempt to better understand this issue and pin point where the problem is. Retrying is a 'get out of jail free' way of handling this problem. But it can mask underlaying issues. Do you believe this could be a provide with the Storage service? (FYI, our application is hosting on Azure WebApps)

XiaoningLiu commented 3 years ago

Not sure but pretty less possbility it's an issue from Storage service. I don't see other reported issues to Stoage service like that.

BenWalters commented 3 years ago

@XiaoningLiu Any news on this one?

XiaoningLiu commented 3 years ago

Not yet. It's still in our backlog and has been triaged in last time. We will keep monitor this and plan with other requests. In the same time, contribtion is welcome.

vishal-rocket commented 3 years ago

image Also frequently receive this error.. its not an enhancement but a critical error please treat it as such... no point using cloud services if they fail on medium size files... use PHP Laravel Forge "microsoft/azure-storage-blob": "1.5.1",