callback passed to exec invoked 2 times, swallows original exception and passes null as input stream

[grunzwei]

Hi,

The code in doConnection has try catch on connectFunc and then on toResponse, but the method toResponse invokes the parser (callback from user), so that's very bad.

Instead of only handling errors during connection - it also handles errors in the user code, without them being aware.

consider the following scenario:

• Connection established correctly, 200
• The parser (not the connection function!) throws an IOException. this gets caught - and disappears. No handling, logging or propagation!
• The parser is invoked once again! This time, the parser is passed the error stream, which is null because the status code was 200. the callback sees 200, handles as success and blows up on null pointer exception.
• The developer is very confused

https://github.com/scalaj/scalaj-http/blob/40c17af9b811064e0708c51991340f2a9a3fe8dd/src/main/scala/scalaj/http/Http.scala#L363

generally speaking, IMO, connectFunc should be wrapped in try catch, the user callback should not have any catch (but finally and stream cleanup is ok).

FYI

[or-shachar]

@hoffrocket - any chance you can validate this issue. We might be able to propose a fix, unless you determine it's a non-issue

[hoffrocket]

Thanks, I can see how this may be a problem. Do you have a reproduction test case and proposed fix? I'm happy to take PRs.

Maybe something like this on the fix side (though I haven't tried to repro myself yet):

        try {
          val stream = try {
            connectFunc(this, conn)
            conn.getInputStream
          } catch {
            case e: java.io.IOException if conn.getResponseCode > 0 =>
              conn.getErrorStream
          } 
          toResponse(conn, parser, stream)
        } finally {
          closeStreams(conn)
        }

[grunzwei]

don't have capacity at this time to dig into this code, it may be good but i remember it was complicated.

we will discuss how and when we or someone from the company can help with this internally. in the meanwhile, an easy workaround is for the consumer to wrap their call back in try catch and wrap all exceptions, thus making sure not to throw a java.io.IOException