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

Any way to rescue the result of a bad redirection? #81

Closed
juanperi opened this issue Feb 23, 2022 · 4 comments
Closed

Any way to rescue the result of a bad redirection? #81

juanperi opened this issue Feb 23, 2022 · 4 comments

Comments

@juanperi
Copy link

Hey!! I was trying your library, and just found this in the wild.
A url returns a redirection to a bad place Location: http://
I thought ok, I will just rescue all possible errors, and log them. But it doesn't work. It seems that this generates an exit that brings down the whole thing.
Do you know of any way of rescuing or stopping this kind of errors, so I can keep processing other records?

Example code:

Mix.install([
  {:req, "~> 0.2.0"},
])
defmodule Test do
  def run() do
    Req.get!("http://www.bugclipper.com")
  rescue
    e -> {:error, inspect(e)}
  end
end

Test.run
@juanperi
Copy link
Author

for the record, this is the return of curl for that url

$ curl http://www.bugclipper.com -v
*   Trying 162.215.226.6...
* TCP_NODELAY set
* Connected to www.bugclipper.com (162.215.226.6) port 80 (#0)
> GET / HTTP/1.1
> Host: www.bugclipper.com
> User-Agent: curl/7.64.1
> Accept: */*
>
< HTTP/1.1 301 Moved Permanently
< Server: nginx
< Date: Wed, 23 Feb 2022 15:40:34 GMT
< Content-Type: text/html; charset=utf-8
< Transfer-Encoding: chunked
< Connection: keep-alive
< X-Frame-Options: GOFORIT
< Location: http://
<

* Connection #0 to host www.bugclipper.com left intact
* Closing connection 0

@wojtekmach
Copy link
Owner

Given Req follows redirects by default and curl does not, an equivalent curl invocation would be:

$ curl http://www.bugclipper.com -v -L
*   Trying 162.215.226.6:80...
* Connected to www.bugclipper.com (162.215.226.6) port 80 (#0)
> GET / HTTP/1.1
> Host: www.bugclipper.com
> User-Agent: curl/7.79.1
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 301 Moved Permanently
< Server: nginx
< Date: Sun, 24 Apr 2022 20:14:41 GMT
< Content-Type: text/html; charset=utf-8
< Transfer-Encoding: chunked
< Connection: keep-alive
< X-Frame-Options: GOFORIT
< Location: http://
<
* Ignoring the response-body
* Connection #0 to host www.bugclipper.com left intact
curl: (3) URL using bad/illegal format or missing URL

and it sensibly errors too.

Since we got an exit from Finch, a standard rescue won't do. You need a catch :exit, reason:

Mix.install([
  {:req, "~> 0.2.0"}
])

defmodule Test do
  def run do
    Req.get!("http://www.bugclipper.com")
  catch :exit, reason ->
    {:error, reason}
  end
end

IO.inspect Test.run()

In theory we could automatically catch exits from Finch but I'm not yet sure if that's a good idea. If anything, I'd like to wait to see more possible failure scenarios.

@wojtekmach
Copy link
Owner

FWIW the bug is not about Req or redirects, it's easy to reproduce it with just Finch:

Mix.install([
  {:finch, "~> 0.11.0"}
])

{:ok, _} = Finch.start_link(name: MyFinch)
Finch.build(:get, "http://") |> Finch.request(MyFinch) |> IO.inspect()
** (exit) :badarg
    (finch 0.11.0) lib/finch/http1/pool.ex:62: Finch.HTTP1.Pool.request/5
    (finch 0.11.0) lib/finch.ex:294: Finch.request/3
    bug.exs:6: (file)

It looks like the exit doesn't originate in Finch, it comes either from NimblePool or more likely from Mint.

The question still stands whether Req should handle exits, but in the meantime I think we can raise a better error from Finch.

@wojtekmach
Copy link
Owner

wojtekmach commented Apr 24, 2022

Closing in favour of sneako/finch#186. If the Finch team would like to move forward with improvements in this area, I'm sure they'd appreciate a PR. Thanks for the report @juanperi!

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

No branches or pull requests

2 participants