Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Load stale data whenever there's any http error #130

Merged
merged 1 commit into from
Dec 12, 2022
Merged

Fix: Load stale data whenever there's any http error #130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Repository/DefaultUnleashRepository.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ public function getFeatures(): iterable
if ($response->getStatusCode() === 200) {
$data = (string) $response->getBody();
$this->setLastValidState($data);
} else {
throw new HttpResponseException("Invalid status code: '{$response->getStatusCode()}'");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is correct. I don't believe PHP is ever going to do the HTTP 304 dance like the other SDKs and anything in the 500 range should be fine to get handled in the catch.

There's two 400 range HTTP status codes I want to clarify:
401 and 429. Do we want to raise error events in the dispatcher when the client is either being denied due to a bad token or rate limited by our infra? I want to say yes, just a sanity check that you've thought about it and agree

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

304 should be handled automatically and shouldn't be any issue.

429 seems like the perfect use-case for this, I'm a little conflicted on the 401, but ultimately I think that the SDK should try to fail as little as possible.

There is a time limit for the stale cache, meaning if the 401 prevails it will fail with an exception eventually, while temporary outages somewhere along the line (like some proxy having an outage) which could cause the 401 will be handled gracefully.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ultimately I think that the SDK should try to fail as little as possible.

Agree with this.

Not critical, but should they body also be supplied in the exception message to help devs debug errors, in case it's available?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eblanshey I don't think it would be useful given the exception is caught immediately so nothing ever propagates further. Or did you mean somewhere else?

Copy link
Member

@sighphyre sighphyre Dec 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

429 seems like the perfect use-case for this, I'm a little conflicted on the 401, but ultimately I think that the SDK should try to fail as little as possible.

Definitely don't think this should cause a failure, idly wondering about the value of being able to respond to a 401 with a hook. Don't think it's harmful to have though so yeah, looks good

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a time limit for the stale cache, meaning if the 401 prevails it will fail with an exception eventually, while temporary outages somewhere along the line (like some proxy having an outage) which could cause the 401 will be handled gracefully.

Not related to this PR so maybe a conversation for a different time and place but is this what we want? My gut feel is the SDK raising an exception almost anywhere is surprising and not really handleable by the caller. Wouldn't mind unpacking this at some point (honestly I'd like a standard set of behaviours for our SDKs on this, which we don't really have right now)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it would be useful given the exception is caught immediately so nothing ever propagates further. Or did you mean somewhere else?

The exception is passed to the FetchingDataFailedEvent, so I'd hook into that event to add a message to our logs. The body would help as much as the header code for context. For example, it could say "Incorrect API key", or "API key expired" or "Account banned", to name a few examples off the top of my head.

But I see this was already merged so I guess we'll leave it at that.

}
} catch (Exception $exception) {
$this->configuration->getEventDispatcherOrNull()?->dispatch(
Expand Down
35 changes: 35 additions & 0 deletions tests/Repository/DefaultUnleashRepositoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,4 +349,39 @@ public function testFallbackStaleCacheNotRefreshing()
sleep(1);
}
}

/**
* @see https://github.com/Unleash/unleash-client-php/issues/129
*/
public function testFallbackStaleCacheNoException()
{
$eventEmittedCount = 0;

$eventDispatcher = new EventDispatcher(new SymfonyEventDispatcher());
$eventDispatcher->addListener(
UnleashEvents::FETCHING_DATA_FAILED,
function (FetchingDataFailedEvent $event) use (&$eventEmittedCount): void {
$event->getException(); // just to cover the line
++$eventEmittedCount;
}
);

$repository = new DefaultUnleashRepository(
new Client([
'handler' => $this->handlerStack,
]),
new HttpFactory(),
(new UnleashConfiguration('', '', ''))
->setCache($this->getRealCache())
->setEventDispatcher($eventDispatcher)
->setTtl(0)
->setStaleTtl(3)
);

$this->pushResponse($this->response);
$this->pushResponse($this->response, 1, 401);
$features = $repository->getFeatures();
self::assertEquals($features, $repository->getFeatures());
self::assertSame(1, $eventEmittedCount);
}
}