thephpleague / flysystem-rackspace

Flysystem Adapter for Rackspace
37 stars 28 forks source link

Expired tokens in long running tasks #22

Open jmserra opened 6 years ago

jmserra commented 6 years ago

Hello,

We're using this wonderful adapter in our workers dispatching queued jobs from different Laravel events.

Lately we're seeing errors in some of those workloads when trying to upload files into the Cloudfiles bucket, those errors come and go, and after talking to the Rackspace team seems there might be some problem with the token expiration.

Rackspace is suggesting to call $client->authenticate() that would refresh the expired token for a new one, but it might be overkill to reauthenticate every single time.

Did you guys got any similar reports about expired tokens in long-lived worker processes ? Any clue to get over it ?

Here's the error trace:

Guzzle\Http\Exception\ClientErrorResponseException: Client error response [status code] 401 [reason phrase] Unauthorized [url] https://storage101.dfw1.clouddrive.com/v1/MossoCloudFS_..../user/123456 in /home/worker/.../vendor/guzzle/guzzle/src/Guzzle/Http/Exception/BadResponseException.php:43 Stack trace:

0 /home/worker/.../vendor/guzzle/guzzle/src/Guzzle/Http/Message/Request.php(145): Guzzle\Http\Exception\BadResponseException::factory(Object(Guzzle\Http\Message\EntityEnclosingRequest), Object(Guzzle\Http\Message\Response))

...

16 /home/worker.../vendor/league/flysystem-rackspace/src/RackspaceAdapter.php(93): OpenCloud\ObjectStore\Resource\Container->uploadObject('user/4564_1...', '\xFF\xD8\xFF\

.....

Thanks!

frankdejonge commented 6 years ago

Hi @jmserra,

I've not run into this specific issue, but I have run into similar ones. My default strategy for these things would be to address this at the boundary level with Flysystem, of course this means such a boundary must be in place for this to have a good effect. Personally I think such cases illustrate nicely how software boundaries help you to deal with accidental complexity. But I'll make it a little more concrete.

For example, if our application was responsible for generating reports of some sort, there'd be a ReportingProcess, which has a Reporter and a ReportsStorage. The ReportStorage would be our boundary to Flysystem. It could look something like:

<?php

class ReportStorage
{
    public function __construct(FilesystemInterface $filesystem)
    {
        $this->filesystem = $filesystem;
    }

    public function storeReport(User $reportIssuer, DateTime $reportedAt, string $report): bool
    {
        $path = "{$reportIssuer->uuid()}-{$reportedAt->format('Y-m-d').txt";

        return $this->filesystem->write($path, $report);
    }
}

If there'd be to be some retry mechanism with the $client->authenticate() call, we could easily do that here. We could even do it pre-emptively.

For example, if we'd have a re-authenticator like this:

class ReAuthenticator
{
    public function __construct($client, int $throttle)
    {
        $this->client = $client;
        $this->throttle = $throttle;
        $this->lastCall = time();
    }

    public function reAuthenticate()
    {
        if (time() > ($this->lastCall + $this->throttle)) {
            $this->client->authenticate();
            $this->lastCall = time();
        }
    }
}

and then our boundary could use that:

<?php

class ReportStorage
{
    public function __construct(FilesystemInterface $filesystem, ReAuthenticator $reAuth)
    {
        $this->filesystem = $filesystem;
        $this->reAuth = $reAuth;
    }

    public function storeReport(User $reportIssuer, DateTime $reportedAt, string $report): bool
    {
        $path = "{$reportIssuer->uuid()}-{$reportedAt->format('Y-m-d').txt";
        $this->reAuth->reAuthenticate();

        return $this->filesystem->write($path, $report);
    }
}

In this case every time the throttle amount has passed we re-authenticate the client before trying to write with Flysystem.

Hope this help!

jmserra commented 6 years ago

Hi @frankdejonge thanks for your extensive reply, it makes total sense, although doing this kind of implementation implies losing the beautiful abstraction Laravel is doing over Flysystem and its different providers through configuration.

So, looking again into the codebase and object structure i'm thinking that for simple scenarios would be easier accessing the client through the Disk object, what about this:

$client = $disk->getDriver()->getAdapter()->getContainer()->getService()->getClient();

if($client->hasExpired())
     $client->authenticate();

Yeah, not the cleanest thing in the world, and it can easily get broken as soon as some of those layers change a little bit.

This would be more elaborated: (still not ideal)

    function reAuthIfNeeded( FilesystemAdapter $disk )
    {
        $driver = $disk->getDriver();
        if(!is_a($driver, \League\Flysystem\Filesystem::class))
            return;

        $adapter = $driver->getAdapter();
        if(!is_a($adapter, \League\Flysystem\Rackspace\RackspaceAdapter::class))
            return;

        $client = $adapter->getContainer()->getService()->getClient();

        if($client->hasExpired())
        {
            $client->authenticate();
        }
    }

At this point im wondering if that should be done transparently by the driver, as if the token is currently expired, shouldn't be its responsibility to update it without the caller even knowing ?

So, when any method of RackspaceAdapter is called, shouldn't first check if the token got expired ?

Thanks again!