Skip to content
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

[flutter_image] Fix : network image memory and nullsafety #8440

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

Conversation

berhili098
Copy link

@berhili098 berhili098 commented Jan 16, 2025

The memory issue arises from _Uint8ListBuilder allocating memory without a defined limit. In scenarios where large or continuous data streams are handled, this can result in excessive memory usage, leading to degraded performance or out-of-memory errors, especially on devices with limited resources.

To address this, the PR introduces a memory usage limit of 50MB. This ensures:

Controlled memory allocation, preventing _Uint8ListBuilder from exceeding reasonable thresholds.
Improved app stability and responsiveness, particularly on low-memory devices or environments with heavy workloads.
Protection against unintended memory leaks or inefficient resource usage.
This change aligns with the broader goal of improving resource management in the flutter_image package, alongside the newly added dispose() method, which cleans up HTTP client resources to prevent potential leaks.

Let me know if further clarification is needed or if you'd like specific examples illustrating the issue!

  • Adds dispose() method to properly clean up HTTP client resources.
  • Adds memory usage limit (50MB) to prevent excessive memory allocation in _Uint8ListBuilder.
  • Improves null safety handling in HTTP request headers.
  • Improves error messages for empty responses and failed requests.
  • Updates documentation to reference latest HTTP RFC.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…ovements

- Added a static `dispose` method to close the HTTP client used for image fetching, preventing potential resource leaks.
- Improved code formatting and consistency in the constructor and method signatures.
- Introduced a maximum size limit for the `_Uint8ListBuilder` to handle large responses more effectively.
- Updated error handling to provide clearer messages for fetch failures.

These changes enhance the reliability and maintainability of the image loading functionality.
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

@berhili098 berhili098 marked this pull request as draft January 16, 2025 11:14
- Bump minimum supported SDK version to Flutter 3.22/Dart 3.4.
- Introduce `dispose()` method for proper HTTP client resource management.
- Implement a 50MB memory usage limit in `_Uint8ListBuilder` to optimize memory allocation.
- Enhance null safety handling in HTTP request headers.
- Improve error messages for empty responses and failed requests.
- Update documentation to align with the latest HTTP RFC.

These changes enhance the reliability and performance of the image loading functionality.
@berhili098 berhili098 marked this pull request as ready for review January 16, 2025 11:20
@stuartmorgan
Copy link
Contributor

Thanks for the contribution!

I'm marking this as a draft pending completion of the checklist; please mark the PR as ready for review once all of the elements of the checklist are complete. When filing the issue, please be sure to include a clear and detailed description of the memory issue you are trying to address, since currently this PR introduces a (incorrectly versioned) breaking change without a clearly described motivation.

@stuartmorgan stuartmorgan marked this pull request as draft January 16, 2025 15:47
@berhili098
Copy link
Author

berhili098 commented Jan 22, 2025

Thanks for the contribution!

I'm marking this as a draft pending completion of the checklist; please mark the PR as ready for review once all of the elements of the checklist are complete. When filing the issue, please be sure to include a clear and detailed description of the memory issue you are trying to address, since currently this PR introduces a (incorrectly versioned) breaking change without a clearly described motivation.

Thank you for the feedback! I’ve completed the checklist, updated the PR description with a detailed explanation of the memory issue being addressed, and reviewed the versioning to align with the changes introduced. The PR is now ready for review. Please let me know if there’s anything else you'd like me to address!

@berhili098 berhili098 marked this pull request as ready for review January 22, 2025 10:21
@berhili098 berhili098 force-pushed the fix/network-images-memory-and-nullsafety branch from 7884eb0 to c894e35 Compare January 22, 2025 10:54
@berhili098 berhili098 force-pushed the fix/network-images-memory-and-nullsafety branch from c894e35 to 04ae85f Compare January 22, 2025 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants