Skip to content

fix improve error handling #4117

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

Open
krushnarout wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix improve error handling #4117

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
767736d
fix improve error handling
krushnarout Jan 7, 2026
f97051a
fix handshake exception
krushnarout Jan 7, 2026
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
9 changes: 7 additions & 2 deletions app/lib/backend/http/http_pool_manager.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import 'dart:async';
import 'dart:io';
import 'package:http/http.dart' as http;
import 'package:flutter/foundation.dart';
import 'package:http/io_client.dart';
import 'package:pool/pool.dart';

Expand Down Expand Up @@ -70,11 +71,12 @@ class HttpPoolManager {
lastError = TimeoutException('Request timeout');
} on SocketException catch (e) {
lastError = e;
} on HandshakeException catch (e) {
lastError = e;
} on http.ClientException catch (e) {
lastError = e;
} catch (e) {
lastError = e;
rethrow;
}

if (i < retries) {
Expand All @@ -83,7 +85,10 @@ class HttpPoolManager {
}

if (lastResponse != null) return lastResponse;
throw lastError ?? Exception('Request failed with unknown error');
if(lastError != null) {
debugPrint('HTTP retry failed: $lastError');
}
return http.Response('', 503);
Comment on lines +88 to +91
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 7, 2026

Choose a reason for hiding this comment

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

critical

While returning a synthetic 503 response prevents an exception from propagating and potentially crashing the app, it silently swallows the underlying error in production builds. The debugPrint call is stripped out in release builds, meaning you will lose all visibility into these failures.

The previous approach of throwing the error was better because it allowed the calling layer (makeApiCall) to catch the specific exception, report it to a crash monitoring service like Crashlytics, and then decide on a graceful failure path (like returning null).

Hiding errors this way can make debugging production issues extremely difficult. If a null response from makeApiCall was causing a crash, the fix should be applied at the call site to handle the null case gracefully, rather than altering the HTTP manager to stop reporting errors.

I recommend reverting this change to ensure failures are properly logged.

    throw lastError ?? Exception('Request failed with unknown error');

Copy link
Member Author

@krushnarout krushnarout Jan 7, 2026

Choose a reason for hiding this comment

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

@gemini-code-assist throwing network errors caused many crashes when the server went offline. Returning a 503 avoids app crashes and unnecessary Crashlytics noise. What approach would you suggest to handle this more safely?

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 7, 2026

Choose a reason for hiding this comment

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

You've raised a very valid point, @krushnarout. The goal is indeed to prevent app crashes due to transient network issues and avoid flooding Crashlytics with 'noise' from expected server unavailability, while still maintaining visibility into these failures for debugging and monitoring.

My previous suggestion to re-throw the error would indeed lead to crashes if not handled robustly upstream, and your current debugPrint approach, while preventing crashes, loses all visibility in production builds.

A safer and more robust approach is to leverage a dedicated error reporting service like Crashlytics, which your project already uses (app/lib/utils/debugging/crashlytics_manager.dart). Instead of re-throwing or silently printing, you can log these network errors as non-fatal events.

This allows you to:

  • Prevent app crashes: The app continues to function gracefully by returning a 503 response.
  • Gain visibility: Errors are logged and accessible in Crashlytics, providing crucial data for understanding network stability and identifying recurring issues.
  • Reduce noise: By marking them as non-fatal, these events won't inflate your crash rate metrics, allowing you to distinguish between true application crashes and expected network interruptions.

I recommend replacing the debugPrint with a call to CrashlyticsManager.recordError, ensuring you pass fatal: false to categorize it correctly. Ideally, the StackTrace captured when the exception was originally caught should be passed for the most accurate debugging information.

Suggested change
if(lastError != null) {
debugPrint('HTTP retry failed: $lastError');
}
return http.Response('', 503);
if (lastError != null) {
CrashlyticsManager.recordError(
lastError,
StackTrace.current, // Consider capturing the original stack trace from the catch block for better context.
reason: 'HTTP retry failed after multiple attempts',
fatal: false, // Mark as non-fatal to avoid crashing the app and reduce noise in crash reports.
);

}

Future<http.StreamedResponse> sendStreaming(http.BaseRequest request) {
Expand Down