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
Close urlConn in StaticFile.fromURL #3625
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good. Optional idea below.
Which idea? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, GitHub ate my comment referenced in the review. It's not a blocker, but just a thought.
@@ -104,7 +105,7 @@ object StaticFile { | |||
) | |||
} else | |||
blocker | |||
.delay(urlConn.getInputStream.close) | |||
.delay(Try(urlConn.getInputStream.close())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we used an old-fashioned try-catch block here, we'd save an allocation for a discarded Try
value, and be able to log a warning on any exceptions. Without seeing what's being thrown, it's hard to tell whether they'd be noise or something worth addressing.
In any case, returning the NotModified
here is correct and a worthwhile improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my perspective something like the following snippet would look nicer here:
blocker
.delay(urlConn.getInputStream.close())
.handleError(_ => ())
.as( ... )
Because it shows intent in a clearer way.
I'm mildly concerned about using Try
just for swallowing exceptions – it is definitely not what it was designed for (even if it does the job).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this looks indeed much nicer.
Previous discussion in #3584
The URLConnection in StaticFile.fromURL is never closed (if the resource is not expired).
This PR reuses the already opened URLConnection to read the response body which in effect causes fs2 to close the URLConnection once the body is fully read.
This PR also wraps
urlConn.getInputStream.close()
in aTry
because it should just be a last attempt to close the resource and should return NotModified in case of errors.Because of the discussion in #3594 I opened this against series/0.21