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

Fixed the stream was not closed when the lottie animation hit the cache #2253

Merged
merged 1 commit into from
May 7, 2023

Conversation

sunlocker
Copy link
Contributor

This PR fixes the stream not closing when the lottie animation hits the cache.

@@ -311,7 +311,7 @@ private static boolean isNightMode(Context context) {
* @see #fromJsonInputStreamSync(InputStream, String, boolean)
*/
public static LottieTask<LottieComposition> fromJsonInputStream(final InputStream stream, @Nullable final String cacheKey) {
return cache(cacheKey, () -> fromJsonInputStreamSync(stream, cacheKey));
return cache(cacheKey, () -> fromJsonInputStreamSync(stream, cacheKey), () -> closeQuietly(stream));
Copy link
Collaborator

Choose a reason for hiding this comment

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

fromJsonInputStreamSync closes the stream here. Is that not sufficient for your use case?

Copy link
Contributor Author

@sunlocker sunlocker Apr 10, 2023

Choose a reason for hiding this comment

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

Thank you for reviewing.

Yes, fromJsonInputStreamSync doesn't close my stream because I set stream and cacheKey via setAnimation(InputStream stream, @Nullable String cacheKey) and hit the cache, but fromJsonInputStreamSync doesn't execute when there is cache. return from here

I also can't close my stream manually because the Lottie framework internally closes my stream via fromJsonInputStreamSync when there is no cache.

Copy link

@ankit-gautam23 ankit-gautam23 left a comment

Choose a reason for hiding this comment

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

I don't think closing stream is needed it comes under capabilities of Lottie framework. It can do it by itself when cache is not there

@sunlocker
Copy link
Contributor Author

sunlocker commented Apr 19, 2023

@ankit-gautam23 Yes, I also think that the Lottie framework should close the stream, but the framework now does not close the stream when there is a cache

@ankit-gautam23
Copy link

Thanks for the Clarification @sunlocker

@sunlocker sunlocker requested a review from gpeal April 20, 2023 11:27
Copy link
Collaborator

@gpeal gpeal left a comment

Choose a reason for hiding this comment

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

Thanks for doing this and sorry it took so long to review.

@gpeal gpeal merged commit 2b80dab into airbnb:master May 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants