clj-commons / aleph

Asynchronous streaming communication for Clojure - web server, web client, and raw TCP/UDP
http://aleph.io
MIT License
2.54k stars 241 forks source link

client: Accept empty file and ensure response on error #589

Closed arnaudgeiser closed 2 years ago

arnaudgeiser commented 2 years ago

Description

In case of TLS usage (ssl-context) and an empty file, a NullPointerException was raised on the following code because bs/to-byte-buffers was returning nil and a manifold.stream/->source cannot be build from it.

(send-streaming-body ch msg
      (-> file
        (bs/to-byte-buffers {:chunk-size (.-chunk-size file)})
        s/->source))

This issue has been reported here : https://github.com/clj-commons/aleph/issues/559

The following PR considers an empty file as a valid request when using TLS (it's already the case when TLS is not used).

Two other changes are included:

Both of approaches have also been considered by @kachayev (#553) but I don't think it will be ever merged...

[1] https://github.com/clj-commons/aleph/pull/553/files#diff-f3c6a88143a34bab27cfd4dde73563c71de85a57da8133def509631b893d05ddR648 [2] https://github.com/clj-commons/aleph/pull/553/files#diff-2d6587452350a3603e05f940d19be2b98e6377dfc7efc3dc1eb099ff332e4b3bR654

KingMob commented 2 years ago

Thanks for working on this. Can you split it up into smaller pieces? It's easier to review when multiple concerns aren't bundled together. I'd prefer to keep the empty file fix and the additional error logging separate.

In particular, there's a lot of old code and notes from Kachayev about errors, and I'd like to look them over.

For the empty file fix, can you add non-TLS tests as well?

arnaudgeiser commented 2 years ago

Thank you @KingMob for the review, I will split this PR in two and add a test regarding non-TLS one. => PR turned into draft