search

Unleash / unleash-client-rust

Unleash client SDK for Rust language projects
Apache License 2.0
23 stars 17 forks source link

Client should handle 502 errors #37

Closed skairunner closed 2 years ago

skairunner commented 2 years ago

Describe the bug

The unleash server can return the following error when polling for features (poll_for_update()): 

\n<html><head>\n<meta http-equiv=\"content-type\" content=\"text/html;charset=utf-8\">\n<title>502 Server Error</title>\n</head>\n<body text=#000000 bgcolor=#ffffff>\n<h1>Error: Server Error</h1>\n<h2>The server encountered a temporary error and could not complete your request.<p>Please try again in 30 seconds.</h2>\n<h2></h2>\n</body></html>\n

The client currently naively tries to convert the returned body into json, fails, then dumps the error and proceeds as if an unrecoverable error happened, by not updating the features until the next poll interval/

This should be easily fixable by inspecting the response's status code and reacting to a 502, before converting it into a json.

Steps to reproduce the bug

Unfortunately I am unsure as to how to reproduce the error, as it may have something to do with specific server configurations or even the environment. However, it might be possible to mock the response from the server.

Expected behavior

Unleash client should probably retry the request as said by the server.

Logs, error output, etc.

No response

Screenshots

No response

Additional context

No response

Unleash version

Client: 0.6.1 alpha Server: 3.17.6

Subscription type

Open source

Hosting type

Self-hosted

SDK information (language and version)

No response

andreas-unleash commented 2 years ago

Hey, thank you for bringing this to our attention. We will raise it internally and get back to you with a response from the team.

In the meantime, can you provide us with some additional information to help reproduce it:

skairunner commented 2 years ago

Upon investigation, the conditions for this error being returned does indeed depend on non-Unleash software: The Unleash server was hosted on a Kubernetes cluster behind a Google load balancer. The server was being CPU-throttled and the load balancer returned the 502 message when it couldn't get a reply from Unleash.

Still, it might be good to have some better handling of server errors.

thomasheartman commented 2 years ago

Hey, @skairunner 👋🏼

Thanks again for raising this! Let me just make sure I get this right:

Are those points correct?

We've talked briefly about this internally, and I think the reason it works the way it does is intentional. There's not much the SDK can do if it receives an error response, and it'll try again after the specified refresh interval anyway. In cases where you need to get updates immediately, you're probably best off bumping up the refresh rate.

Does that sound reasonable to you?

Still, it might be good to have some better handling of server errors.

That's a fair point! What would you classify as "better handling" in this case? What would you expect? ☺️

skairunner commented 2 years ago

After learning that it was not an Unleash server thing but rather from the load balancer, I think my ticket might be fine being closed 😅 If it was inherently part of the unleash api then the client definitely would need to handle it, but since it's not...

The client doesn't currently log the 502 error at all: it simply says poll: failed to retrieve features; with PR #36 it says poll: failed to retrieve features - Error("expected value", line: 2, column: 1) which is not incredibly helpful either. In the project I'm working on, I added a small hack to print the actual json value that failed to parse which is how I discovered the issue. So I guess what is really needed is improved error reporting.

Please feel free to close this issue!

thomasheartman commented 2 years ago

Got it! In that case I'll close this for now ☺️ I think getting some more information would be good though; maybe it's something that can be worked into #36?