Consider adding `Content-Type: text/plain` to HTTP 500 response in `org.httpkit.server.HttpHandler#run` about http-kit HOT 6 CLOSED

@sumbach Thanks for pinging about this Sam 👍

Have just added 36c8cdf - please take a moment to double check if you're happy with the approach there?

Specifically, we had a failing assert (which throws AssertionError). It looks like :pre function preconditions also throw AssertionError as well.

Yes, this is unfortunately a bit of a common gotcha. Contrary to popular advice, I generally prefer catching Throwable for this reason. And it's also one of the reasons that I prefer throwing ExceptionInfos for Truss, etc.

from http-kit.

One comment on the approach: You could allocate and initialize the HeaderMap in an instance-level (or class static) constant.

I was thinking about the possibility of catching OutOfMemoryError, in which case you'd want to avoid allocations along this code path. But it looks like HttpEncode has to allocate ByteBuffers (among other things), so there's probably no benefit to avoiding this HeaderMap allocation "just in case" we get an OOM.

That said, a constant HeaderMap (or dedicated factory function / constructor) could make this code marginally more readable:

cb.run(HttpEncode(503, ContentTypeTextPlain, "Server unavailable, please try again", this.serverHeader))

from http-kit.

One comment on the approach: You could allocate and initialize the HeaderMap in an instance-level (or class static) constant.

Done, thanks Sam 👍 I've just released v2.8.0-RC1 which includes this work.

Cheers!

from http-kit.

Thank you, @ptaoussanis 😀 👍

from http-kit.

I only noticed the "missing" Content-Type header because my ring middleware is only catching Exception and some non-Exception throwables were sneaking through to org.httpkit.server.HttpHandler#run.

Specifically, we had a failing assert (which throws AssertionError). It looks like :pre function preconditions also throw AssertionError as well.

Arguably, these should throw some appropriate Exception instead of AssertionError (which should be reserved for coding errors), but it's not implausible to encounter a non-Exception in the wild -- maybe IOError?

from http-kit.

✨✨ @ptaoussanis ✨✨

This is amazing!! I just tested with a local http-kit build (commit e5b69e1) and confirmed that my errant AssertionError now yields HTTP 500 with Content-Type: text/plain; charset=utf-8👍 💯 😀

The advice I've heard against catching Throwable is that it's often pointless to catch and attempt to handle many JVM-initiated exceptions (like OutOfMemoryError) -- probably better to let the thread, and perhaps even the whole JVM process, crash on "unrecoverable" exceptions.

But when user code throws non-Exception, how can we distinguish between these and "unrecoverable" exceptions? I don't think we have a better choice than catching Throwable.

For my part, I will avoid assert (and defn+:pre?) for anything related to user input --
I'll take your suggestion to throw some appropriate unchecked RuntimeException (like ExceptionInfo).

from http-kit.