Can't retrieve response body with :as :stream, :throw-exception? true

[aeriksson]

So if I invoke a command with both :as :stream and :throw-exception? true, there seems to be no way to read the response body. For instance, doing:

(try (docker/invoke client {:op :ContainerArchive
                            :params {:id "foo", :path "/not-a-path"}
                            :as :stream
                            :throw-exception? true})
     (catch Exception e (:body (ex-data e))))

just gives me something like

#object[okio.RealBufferedSource$inputStream$1 0x743ab33f "buffer(ResponseBodySource(okhttp3.internal.http1.Http1ExchangeCodec$ChunkedSource@3c34c8f2)).inputStream()"]

while

(try (docker/invoke client {:op :ContainerArchive
                            :params {:id "foo", :path "/not-a-path"}
                            :as :stream
                            :throw-exception? true})
     (catch Exception e (slurp (:body (ex-data e)))))

throws java.io.IOException: closed.

There should be some way to read the response body when things fail.

[lispyclouds]

Hey @aeriksson thanks for this! The idea I was having is this is akin to calling a function which could possibly throw, so either the return value or the exception is the outcome of the call. But yes this is an interesting case and I can think of maybe wrapping the return as part of the exception somehow.

Also could you describe me a scenario where/how you could use the stream(or any other value) when an exception is thorwn? This would help me think of a better solution. And yes, any and every help is much welcome! 😄

[aeriksson]

Sure! I'm copying files to/from Docker via :PutContainerArchive/:ContainerArchive.

I want to:

• Stream files to/from Docker without having to convert them to strings (so I need :as :stream).
• Handle any errors that might pop up (e.g. due to missing files or missing container), without having to consume the stream (so I need :throw-exception? true).

The problem here is that I'm currently only able to see the response status code in the thrown exception, whereas I'd like to be able to see the response body as well (so I can log and handle the error appropriately).

One way to solve this that would make me happy at least is if you would just slurp the response body before throwing the exception (I'll have to parse the response body either way when there's a failure, so it's fine if you slurp it — AFAICT Docker errors are normally pretty small anyway, so the overhead should be minimal).

[lispyclouds]

@xsc for this issue would slurping and assoc-ing the body from the inputstream here https://github.com/into-docker/unixsocket-http/blob/master/src/unixsocket_http/core.clj#L61 would be a good solution? I can raise a PR if you think so?

[xsc]

@lispyclouds @aeriksson Wouldn't the following address the use case?

(let [{:keys [status body]}
       (docker/invoke client {:op :ContainerArchive
                              :params {:id "foo", :path "/not-a-path"}
                              :as :stream
                              :throw-exception? false})]
  (if (>= status 400)
    (with-open [in body]
      (let [error-data (slurp in)]
        ...)) ;; error!
    ...)) ;; success!

At least that's how I imagine it from unixsocket-http's perspective. Not sure if there is anything in clj-docker-client that prevents that from working.

[lispyclouds]

This works too! I guess @aeriksson 's control flow was that an exception was raised?

[aeriksson]

@xsc yeah that works too — I didn't realize I could check the status without consuming the body. Regardless, I think something like @lispyclouds's suggestion would be good — it's a bit counterintuitive that the body can't be accessed when an exception is thrown and :as :stream is set.

[xsc]

@aeriksson I think there is a high risk that people would forget to close the stream if it was kept open when an exception is thrown. I'd argue that it's not good practice to leave exception cleanup to the user. For the alternative, I also think it's not good practice to consume the stream into memory. I know I'm constructing a case here, but what if the response has status 404 and 1TB of debug data in the body? 💥

This is my view for unixsocket-http which could be used to communicate with any kind of endpoint, so I would not change the current behaviour there. (I do agree, though, that having the :body in the exception but not being able to read it is a bit problematic.)

For Docker, the endpoints and amounts of data are known, so there is no actual risk. Additionally, I'm not sure my snippet of code up there actually works since clj-docker-client does directly return the :body.

So, potential options:

1. Implement the non-closing behaviour in clj-docker-client, with the potential risk that entails.
2. Adjust the API (or both APIs) to make the behaviour explicit, e.g.:
   • {:as :stream, :close-on-exception? false}
   • {:as :stream, :throw-exception? :throw-and-keep-open}
   • {:as :stream, :on-exception (fn [response] ...)}
   • ...
3. Follow clj-http's lead and don't include the response in the exception at all, unless :throw-entire-message? true is set; in which case it's a conscious decision by the user and we could leave the stream/socket open.

[lispyclouds]

@xsc very fair points and I too would think the clj-http path would be better both in terms of user experience, keeping things from blowing up automatically and being explicit.

[xsc]

[unixsocket-http "1.0.7"] now has :throw-entire-message?. The behaviour is effectively backwards-compatible, since exceptions still contain the response, only for stream/socket responses the :body is closed and removed unless the flag is set.

Edit: While testing into-docker, I found a possible regression. Please wait until using the new version. :/
Edit2: [unixsocket-http "1.0.8"] is ready to use!

[lispyclouds]

Implemented in 1.0.3

[lispyclouds]

https://github.com/into-docker/clj-docker-client#reading-a-streaming-output-in-case-of-an-exception-being-thrown