-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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.
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
No Native HTTP Retry Mechanism in Apps-Engine #34872
Comments
Please complete the server setup info. Thanks. |
Hey @reetp, I was talking with @jannatkhandev about his idea of having a native HTTP retry mechanism so I asked @jannatkhandev to create an issue so we could discuss if we agree with his idea while having more visibility with the rest of the engineering team. |
This comment was marked as off-topic.
This comment was marked as off-topic.
Hey @jannatkhandev , thanks for raising this point. I do think, though, that the approach you propose is way larger and more opinionated than it needs to be to solve this problem. For instance, I don't think we should automatically provide settings in the app to toggle retrying on and off - this should be a decision the app developer should make, not the user. A suggestion I'd give in this scenario would be to export a function that would do the retrying, something along the lines of this library https://www.npmjs.com/package/exponential-backoff - it has the plus side of being agnostic to the task it's executing (not tied to HTTP) while being as flexible with the configuration as your proposal. Something to note is that apps can already include the library above to implement retries. It would be useful to provide a function like that from the apps-engine if we were to integrate with some mechanism like logging or scheduling, for instance, by allowing the retries to be non-blocking. Do you think that doing so would solve the problem as you see it? |
Hey @d-gubert Thank you for the thoughtful feedback! I agree that my proposal is kinda overly complex and opinionated, my idea was giving admin to control whether retries should be enabled on Http calls made by the app. [this setting would be available if the app developers extends configuration to add a new Retry class implementation, similar to how settings are handled in the OAuth2Client implementation] so now in order to proceed, instead of the full provider implementation, we could add a simple retry utility function to the engine that:
Something like?: export interface IRetryOptions {
maxAttempts ? : number;
backoff ? : (attempt: number) => number;
shouldRetry ? : (error: any) => boolean;
}
export async function withRetry < T > (
operation: () => Promise < T > ,
options: IRetryOptions = {},
logger ? : AppConsole
): Promise < T > {
const {
maxAttempts = 3,
backoff = (attempt) => Math.min(1000 Math.pow(2, attempt), 10000),
shouldRetry = () => true
} = options;
let lastError: any;
for (let attempt = 0; attempt < maxAttempts; attempt++) {
try {
return await operation();
} catch (error) {
lastError = error;
if (!shouldRetry(error) || attempt === maxAttempts - 1) {
throw error;
}
const delay = backoff(attempt);
logger?.debug(Retry attempt $ {
attempt + 1
}
/${maxAttempts} after ${delay}ms);
await new Promise(resolve => setTimeout(resolve, delay));
}
}
throw lastError;
} I'd be really grateful if you can nudge me in correct direction, will then take forward as advised. |
Yup, I think that direction is the way to go |
No Native HTTP Retry Mechanism in Apps-Engine
Current Situation
Currently, the Rocket.Chat Apps-Engine lacks a native retry mechanism for HTTP requests. This means that apps need to implement their own retry logic when dealing with unreliable external services or temporary network issues, leading to:
Proposed Solution
Implement a new
RetryHttpProvider
class that can be optionally enabled by apps to provide robust retry functionality without affecting existing apps or adding overhead to the base implementation.Implementation Details
1. Create RetryHttpProvider Class
2. Create Configuration Manager
3. Usage in Apps
Benefits
1. Zero Overhead for Existing Apps
Http
class2. Opt-in Enhancement
RetryHttpConfiguration
3. Consistent Implementation
Implementation Steps
Core Changes
IHttpRetryConfig
interface to define retry configurationRetryHttpProvider
class extending baseHttp
RetryHttpConfiguration
for settings managementAccessor Manager Modifications
This is a critical part of the implementation that enables the override functionality:
How the Override Works
AppAccessorManager
is responsible for providing HTTP instances to appsgetHttp()
:RetryHttpProvider
instanceHttp
instancehttpProviders
map for subsequent requestsThis approach ensures:
Integration
AppAccessorManager
for HTTP provider creationTesting
Migration Guide
For apps wanting to use the retry functionality:
RetryHttpConfiguration
Server Setup Information:
Client Setup Information
Additional context
Relevant logs:
The text was updated successfully, but these errors were encountered: