Skip to content

Close a cancelled request's stream off the calling thread - #2248

Draft
mpretty-cyro wants to merge 1 commit into
devfrom
fix/giphy-cancel-close-off-main
Draft

mpretty-cyro wants to merge 1 commit into
devfrom
fix/giphy-cancel-close-off-main

Conversation

@mpretty-cyro

Copy link
Copy Markdown
Collaborator

Contributor checklist

  • I have tested my contribution on these devices:
  • Virtual device: Pixel 6 profile, API 37 (arm64), play flavour, debug
  • My contribution is fully baked and ready to be merged as is
  • I ensure that all the open issues my contribution fixes are mentioned in the commit message of my first commit using the Fixes #1234 syntax

Description

Crash cluster: NetworkOnMainThreadException in okhttp3.internal._UtilJvmKt.closeQuietly, from
GiphyAdapter.onViewRecycled / onStop → Glide RequestManager.clear → ChunkedImageUrlFetcher.cancel →
CallRequestController.cancel → Util.close. Closing the TLS response body can write to the socket.

CallRequestController.cancel() now closes the stream on SignalExecutors.UNBOUNDED.

Testing: compiled. Tried to reproduce on dev on an emulator with a throttled network (scrolling the grid and leaving the picker mid-download); it didn't reproduce, so this is untested beyond the normal picker flow.

Glide cancels image requests on the main thread, for example when the
Giphy grid recycles a cell while scrolling or when the activity stops.
CallRequestController.cancel() then closed the response body there, and
closing a TLS stream can write to the socket, which threw
NetworkOnMainThreadException.

@mpretty-cyro mpretty-cyro left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

lgtm

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.

1 participant