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

Default rejected-handler will leak buffers in raw stream HTTP servers #693

Open DerGuteMoritz opened 8 months ago

DerGuteMoritz commented 8 months ago

Problem

Given an HTTP server started with :raw-stream? true, when aleph.http.server/handle-request hits the RejectedExecutionException path, the default handler (i.e. when no custom rejected-handler was provided) will leak any byte buffers which are already present in the request's :body stream.

Possible solutions

  1. Drain the body channel and release all byte buffers still contained in it (does that cover all bases?)
  2. Document this fact (as suggested by @arnaudgeiser here)
  3. Require passing a custom rejected-handler when :raw-stream? true is passed
KingMob commented 8 months ago

I think if there's no user-supplied raw error handler, we should supply a default in raw-ring-handler. That default error handler could then close the stream, ms/consume all the ByteBufs and .release() them, as well as send a 503.

Hmmmm. We could also wrap a user-supplied error-handler with one which checks the refcnt and releases only if > 0. Theoretically this would be safe and helpful to users... But could there be a race if the user releases it, the ByteBuf goes back to the pool, and some non-netty thread grabs it? Maybe not.

I think you two know this area better than I, so I defer to your judgment.