search

wechaty / token

Wechaty Token-Based Authentication Manager
https://www.npmjs.com/package/wechaty-token
Apache License 2.0
2 stars 2 forks source link

Retry when 0.0.0.0 host was returned #2

Closed hcfw007 closed 2 years ago

hcfw007 commented 2 years ago

https://github.com/wechaty/token/blob/184732171b0b23bf01b7927cc2a2d7ddc3d5591b/src/retry-policy.ts#L18-L23

From this retry policy we can see that token discovery will retry only when there is an Error trying to reach token authority. However a '0.0.0.0' ip should also trigger retry because the puppet service could possibly be starting. And from the discoveryApi code there is also no special handler for '0.0.0.0' address.

https://github.com/wechaty/token/blob/47db65a7d42d6d9ca30cd3b6032ffd55c6448a07/src/wechaty-token.ts#L90-L121

I think we should add this retry mechanism to '0.0.0.0' results.

huan commented 2 years ago

Have you upgraded the server code to the latest? If not, please upgrade the server code first.

The protocol currently will retry when there's a HTTP 404.

The old server protocol will use 200 with a 0.0.0.0 IP, which has been deprecated.

The new server will response HTTP 404 for not found and will be correctly retried.

Please let me know if it make sense to you.

hcfw007 commented 2 years ago

I tried, and it did not retry. I think you should not resolve(undefined) when you got 404 status code. Instead, you should

return reject(new Error('WechatyToken discoverApi() got 404')

Also we shoud add a 5-10 seconds delay for retry.

huan commented 2 years ago

Please feel free to add an unite test to fail the current code with your expected behavior.

Then we will continue discussing based on that.

hcfw007 commented 2 years ago

https://github.com/wechaty/token/blob/184732171b0b23bf01b7927cc2a2d7ddc3d5591b/src/wechaty-token.spec.ts#L115-L170 You have a test for 404 already and seems return undefined and not retrying is the intended behavior.

huan commented 2 years ago

Glad to know that you have read the existing unit tests!

hcfw007 commented 2 years ago

So not retrying is intended instead of a bug? I think that's a bit odd since if we start a bot and puppet server at the same time, it's quite normal for the discovery to get a 404 response at fist.

huan commented 2 years ago

So not retrying is intended instead of a bug?

I'm not saying that. There are many potential use cases and they might be valid.

The current design is just the first version, and I think it has followed the below two principles:

  1. If the token is 404: it should throw an error out because the discover service said the token is invalid (not exist at all)
  2. If the token is 200: it should have a retry algorithm because the discover service said the token is valid.

For your case, I think you should design some solutions and we can have a future discussion based on your analytics.

For example, I think there will be very easy to help the puppet service client to start AFTER the puppet service server has successfully registered to the discovery service, you can enforce this order easily by some technic, then problem solved.

Or, if you can catch the 404 when starting the Wechaty, then you can implement your own retry algorithm with the Wechaty API layer.

And of course, you can propose a proposal to argue that the wechaty-token module should have a retry algorithm when the token is 404, and I'd love to read your analytics about that.

Thank you very much.