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

Fixes #3565 Handle FileNotFoundException in StaticFile.fromURL #3594

Closed
wants to merge 5 commits into from
Closed

Fixes #3565 Handle FileNotFoundException in StaticFile.fromURL #3594

wants to merge 5 commits into from

Conversation

ashwinbhaskar
Copy link
Collaborator

@ashwinbhaskar ashwinbhaskar commented Jul 19, 2020

No description provided.

@ashwinbhaskar ashwinbhaskar changed the title [WIP] Fixes #3565 Handle FileNotFoundException in StaticFile.fromURL Fixes #3565 Handle FileNotFoundException in StaticFile.fromURL Jul 19, 2020
))
Try(url.openStream()).fold(
fa = {
case _: FileNotFoundException => None
Copy link
Contributor

@satorg satorg Jul 19, 2020

Choose a reason for hiding this comment

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

I guess this may be unsafe – if it happens that openStream will throw some other exception, then we get MatchError here.

Copy link
Collaborator Author

@ashwinbhaskar ashwinbhaskar Jul 20, 2020

Choose a reason for hiding this comment

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

@satorg I don't think there is a way to have utl.openStream in F.delay. For example if I put url.openStream in F.delay

val r: F[Option[Stream[F,Byte]]] = F.delay(url.openStream)
            .redeem(recover = {
              case _: FileNotFoundException => None
            }, f = {inputStream => Some(readInputStream[F](F.pure(inputStream), DefaultBufferSize, blocker))})

Notice that the type of r is wrapped in a F. But we want a result of the type Option[Response[_][F]].

Copy link
Contributor

@satorg satorg Jul 20, 2020

Choose a reason for hiding this comment

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

@ashwinbhaskar it should be almost always possible :)
This implementation works for me (as well as against the test case in your PR):

  def fromURL[F[_]](url: URL, blocker: Blocker, req: Option[Request[F]] = None)(implicit
      F: Sync[F],
      cs: ContextShift[F]): OptionT[F, Response[F]] = {
    val fileUrl = url.getFile()
    val file = new File(fileUrl)
    OptionT.apply(F.suspend {
      if (file.isDirectory()) // TODO: this call should be suspended in the blocker
        F.pure(None)
      else {
        val urlConn = url.openConnection
        val lastmod = HttpDate.fromEpochSecond(urlConn.getLastModified / 1000).toOption
        val ifModifiedSince = req.flatMap(_.headers.get(`If-Modified-Since`))
        val expired = (ifModifiedSince, lastmod).mapN(_.date < _).getOrElse(true)

        if (expired) {
          val lastModHeader: List[Header] = lastmod.map(`Last-Modified`(_)).toList
          val contentType = nameToContentType(url.getPath).toList
          val len = urlConn.getContentLengthLong
          val lenHeader =
            if (len >= 0) `Content-Length`.unsafeFromLong(len)
            else `Transfer-Encoding`(TransferCoding.chunked)
          val headers = Headers(lenHeader :: lastModHeader ::: contentType)

          blocker
            .delay(url.openStream)
            .redeemWith[Option[Response[F]]](
              {
                case _: FileNotFoundException => F.pure(None)
                case other => F.raiseError(other)
              },
              stream =>
                F.pure(
                  Some(
                    Response[F](
                      headers = headers,
                      body = readInputStream[F](stream.pure[F], DefaultBufferSize, blocker))))
            )
        } else
          blocker
            .delay(urlConn.getInputStream.close())
            .as(Some(Response(NotModified)))
      }
    })
  }

Feel free to borrow any part of this snippet if you'd like to.

Copy link
Collaborator Author

@ashwinbhaskar ashwinbhaskar Jul 21, 2020

Choose a reason for hiding this comment

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

ah..I see the suspend did the trick:)

headers = headers,
body = readInputStream[F](F.delay(url.openStream), DefaultBufferSize, blocker)
))
Try(url.openStream()).fold(
Copy link
Contributor

@satorg satorg Jul 19, 2020

Choose a reason for hiding this comment

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

I wonder if we really need Try here since we already have F: Sync around and can suspend effects in it and do all necessary error handling inside?

Copy link
Collaborator Author

@ashwinbhaskar ashwinbhaskar Jul 21, 2020

Choose a reason for hiding this comment

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

Fixed

@dubinsky
Copy link

dubinsky commented Jul 19, 2020

@ashwinbhaskar I tried out your changes in my local copy of the code, and it works great!
I'll be tracking the changes to this pull request as it gets refined, and then switch to the official code when it gets released.
Thank you so much!

@dubinsky dubinsky mentioned this pull request Jul 19, 2020
4 tasks
.fromURL[IO](new URL("https://github.com/http4s/http4s/fooz"), testBlocker)
.value
.unsafeRunSync
s must_== None
Copy link
Contributor

@satorg satorg Jul 20, 2020

Choose a reason for hiding this comment

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

Not critical at all, but I'd suggest using s must beNone here.

Copy link
Collaborator Author

@ashwinbhaskar ashwinbhaskar Jul 21, 2020

Choose a reason for hiding this comment

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

done and also added one more test to check the case where an exception is raised.

urlConn.getInputStream.close()
Some(Response(NotModified))
}
F.delay(url.openStream)
Copy link
Contributor

@satorg satorg Jul 21, 2020

Choose a reason for hiding this comment

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

I think it makes sense to delay url.openStream (which is apparently a blocking operation) in the blocker as well as urlConn.getInputStream.close below, doesn't it?

Copy link
Collaborator Author

@ashwinbhaskar ashwinbhaskar Jul 21, 2020

Choose a reason for hiding this comment

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

true. made the change and pushed

s must beNone
}

"raise exception when url does not exist" in {
Copy link
Contributor

@satorg satorg Jul 21, 2020

Choose a reason for hiding this comment

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

This case is a really good catch! The only thing I could mildly recommend is to try leveraging the throwA matcher that should make the following check as simple as

StaticFile
          .fromURL[IO](new URL("https://quuzgithubfoo.com/http4s/http4s/fooz"), testBlocker)
          .value
          .unsafeRunSync must throwA[UnknownHostException]

https://etorreborre.github.io/specs2/guide/SPECS2-4.10.0/org.specs2.guide.Matchers.html#out-of-the-box

Copy link
Collaborator Author

@ashwinbhaskar ashwinbhaskar Jul 21, 2020

Choose a reason for hiding this comment

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

makes sense. made the change and pushed

case _: FileNotFoundException => None
case other => throw other
},
f = { inputStream =>
Copy link
Contributor

@satorg satorg Jul 21, 2020

Choose a reason for hiding this comment

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

Was struggling a bit to understand what is f here? Then realized that it is just the redeem function's parameter name :)

satorg
satorg approved these changes Jul 21, 2020
Copy link
Contributor

@satorg satorg left a comment

Looks good to me.

hamnis
hamnis approved these changes Jul 24, 2020
@satorg
Copy link
Contributor

satorg commented Jul 29, 2020

Fixes #3565 (just to let GitHub create a link to the original issue)

@rossabaker
Copy link
Member

rossabaker commented Aug 5, 2020

Sorry, still catching up from my vacation.

My question is whether we want this on the 0.21 series? It's a semantic change, but is anyone intentionally relying on a FileNotFoundException bubbling up as a 500?

@rossabaker rossabaker added this to the 0.21.7 milestone Aug 5, 2020
@satorg
Copy link
Contributor

satorg commented Aug 5, 2020

I doubt if we can know it for sure. But I guess even if someone relies on FileNotFoundException, they still need to handle a None return value as well. Therefore this change should not break someone's code completely.

@rossabaker
Copy link
Member

rossabaker commented Aug 5, 2020

I think you're right. I don't like making changes that don't cause type errors, but this is a much more sensible default. Let's cherry-pick it.

@rossabaker
Copy link
Member

rossabaker commented Aug 6, 2020

Picked onto series/0.21 as c5903bd. Thanks!

@rossabaker rossabaker closed this Aug 6, 2020
@rossabaker rossabaker added enhancement Feature requests and improvements retarget Cherry-pick or reopen on another branch labels Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and improvements retarget Cherry-pick or reopen on another branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants