Skip to content

Drain socket before throwing #79289

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

Merged
merged 1 commit into from
Mar 30, 2021
Merged

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Mar 29, 2021

Fixes #78741
Similar to #78742

This makes it so that when a network image fails with a non-OK status code, we drain the socket to avoid keeping it open longer than necessary. Updates an existing test with this expectation.

Note that drain is documented as doing the same as what listen did in #78742, but just seems a bit more idiomatic to me.

Rather than creating a completely new test, I added an expectation to an existing test that exercised this code branch.

/cc @alexd6631

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Mar 29, 2021
@google-cla google-cla bot added the cla: yes label Mar 29, 2021
@dnfield dnfield added the a: images Loading, displaying, rendering images label Mar 29, 2021
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@alexd6631
Copy link

Hi @dnfield,

I agree that drain feels more idiomatic, I hesitated to use it in my previous MR.
Just a slight difference to note, is that in #78742 I used listen in order to explicitly silence errors that could arise when draining the response. With drain() I think an error will be thrown while awaiting the future, discarding the NetworkImageLoadException.

It may not be that important in practice, but I think this small behavior difference should be considered.

Best regards

@dnfield
Copy link
Contributor Author

dnfield commented Mar 29, 2021

@alexd6631 I think that's ok. We're only throwing here to get into the evict logic in the catch block below. If there's truly a socket exception involved, then it'd be better to throw that more specific error than the generic one we're throwing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: images Loading, displaying, rendering images framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Socket leak in network image IO (iOS)
5 participants