dream11 / kong-circuit-breaker

Kong plugin for wrapping all proxy calls with a circuit-breaker
https://dream11.github.io/kong-circuit-breaker/
MIT License
36 stars 9 forks source link

api_call_timeout_ms has problem #21

Closed 18wu closed 2 years ago

18wu commented 2 years ago

Summary

If a timeout is triggered,when I disabled this plugin,it also say "The upstream server is timing out". Very urgent

Steps To Reproduce

1. 2. 3. 4.

Additional Details & Logs

chirag-manwani commented 2 years ago

@18wu Could you describe the issue a bit more clearly? What is the current behavior? What is the expected behavior? What version of the Plugin are you using? Do you have any Kong logs?

18wu commented 2 years ago

the timeout config : image but when I delete the plugin or disabled the plugin, the meaasge still shows "timeout". I see the code is: image maybe that's the problem. the plugin version is the lastest!

chirag-manwani commented 2 years ago

Could you also check the timeout value for the Service Object? It is possible that the Upstream Server is taking too long to respond and in that time Kong is timing out the request (which is just an HTTP Client thing, nothing to do with circuit breaker or Kong).

18wu commented 2 years ago

the timeout value for the Service Object : image default 60000ms. The problem is that this plugin modified the timeout configuration, but it did not change the configuration back after closing the plugin.

chirag-manwani commented 2 years ago

I am not sure if that is the case since ngx.ctx is a per request table, So it needs to be set for each request. Nevertheless I will try to replicate it on my end. Could you help me out with the plugin version you are using?

18wu commented 2 years ago

v2.1.0

chirag-manwani commented 2 years ago

@18wu When you first set (or make any subsequent changes) to the timeout value, the timeout gets reflected in the 2nd request. The first request you hit after making the changes uses the old timeout value which is cached at Kong level. Let's say-

  1. You configure a circuit breaker plugin instance on a route /abc with timeout set to 50ms. If you send a request now the timeout value used will not be 50ms but the read timeout value set at the service level.
  2. Hit another request and this time the timeout value will be 50ms.
  3. Change timeout to 1ms so that request is confirmed to time out.
  4. Hit another request but it won't timeout since 50ms value is cached.
  5. Hit another request and this will timeout. And so on. TLDR Updated values will reflect from 2nd request onwards. You can confirm this by logging ngx.ctx, there you will find a "balancer data" key which has a table value which has timeout values.
chirag-manwani commented 2 years ago

If you still face issues please post the steps to replicate the issue.

18wu commented 2 years ago

I know what you mean,You mean that the timeout of the plugin is set, and the first time will not take effect.But my problem is that I deleted the circuit breaker plugin and the request I sent still timed out.

chirag-manwani commented 2 years ago

@18wu I was able to replicate this and it is indeed a problem. But for now, I can suggest a workaround that involves enabling a circuit breaker plugin globally which has the default timeout of 60000ms set (or any other value you may like). Meanwhile, I am also looking at how to fix this issue. I encourage you to find a fix as well and contribute if possible.

18wu commented 2 years ago

ok,thanks

chirag-manwani commented 2 years ago

@18wu I looked over Kong's source code to understand why request level data (kong.ctx) is somehow propagating to subsequent requests. Turns out that ngx.ctx has a reference to the service and route objects, and if we modify any property for these objects, that change persists for subsequent requests(In our case ngx.ctx.service.read_timeout)

The actual value used for setting timeouts is ngx.ctx.balancer_data Refer source code. Our code was working since balancer_data is created per request using the service level config, Refer source code.

So if we just change ngx.ctx.service.read_timeout to ngx.ctx.balancer_data.read_timeout, the issue will be solved. Would you be willing to contribute to this change? Along with test cases if possible?

18wu commented 2 years ago

image

https://user-images.githubusercontent.com/62932262/157205317-bcb6e08e-4112-4b44-8c1c-58ddb9c01cd6.mp4

I changed ngx.ctx.service.read_timeout to ngx.ctx.balancer_data.read_timeout,now the call is like this.

chirag-manwani commented 2 years ago

I can't see anything in the video, it's just a black screen. Could you repost it or maybe describe the issue?

18wu commented 2 years ago

image download it. I can't describe it accurately, which means that I asked for it 10 times, five times will be Timeout, five times will be normal, and it will always be Timeout until it is modified.

chirag-manwani commented 2 years ago

What is the api_call_timeout_ms value?

18wu commented 2 years ago

2ms

18wu commented 2 years ago

And I found that now the plugin is enabled, which is also the above situation.

chirag-manwani commented 2 years ago

So 2ms is a very small value, and if for any reason the app or Kong takes longer than that, you'll get a timeout. However, if you set the value to 2ms, and then remove the plugin, the timeout value should fall back to the timeout configured on the service. Which I think solves your issue, right?

18wu commented 2 years ago

Now the problem is changed to ngx.ctx.balancer_data.read_timeout, but it seems useless. Can you try it yourself?

chirag-manwani commented 2 years ago

I tried it out on local and it works for me. If I set the timeout to say 100ms, and the API takes more than that, Kong returns a timeout error. And if I remove the plugin, and service timeout is set to say 60000ms (default), then the request does not time out, and Kong returns a proper response. If you remove the circuit breaker plugin and set the value to 2ms at the service level, you will face the same issue(some requests will pass and some will not) which is fine since the upstream or Kong might take more time than that. But this change fixes your earlier issue, which was the timeout issue even after deleting the plugin.

18wu commented 2 years ago

ok,Thank you very much.

abstulo commented 2 years ago

Any updates on when the above mentioned fix will be introduced in a new release of the plugin?

chirag-manwani commented 2 years ago

Will push the fix in a couple of days.