Add option for generating response for timeouts and errors in async handlers

[mourjo]

When timeout values are non-zero in async handlers, provide a way to generate the response that is sent back when timeouts occur. Expose an optional parameter :async-timeout-handler which expects a Ring-style request map and should generate a ring-style response map.

Similarly add an option to generate a ring-style response map for handling errors in async handlers.

[weavejester]

Could you expand upon why the timeout handler should be part of the adapter, rather than as middleware?

What sort of errors do you expect the error handler to intercept?

Also please ensure your commits adhere to the contributing guidelines of the project.

[mourjo]

Hey thanks for the quick response! Answering your questions:

Could you expand upon why the timeout handler should be part of the adapter, rather than as middleware?

I am not sure how to put the timeout handling inside a middleware because when the request times out, the response is sent by Jetty (depending based on the listeners on the AsyncContext) https://docs.oracle.com/javaee/7/api/javax/servlet/AsyncContext.html It might be possible to launch a future from the middleware and use deref with timeout there, but then the async nature and :timeout key in the server setup won't play along, right?

What sort of errors do you expect the error handler to intercept?

I saw that the adapter calls sendError so the error response is generated by Jetty and isn't following the Ring semantics, but I can revert this change.

Also please ensure your commits adhere to the contributing guidelines of the project.

Sure thing, I thought I had taken care of this, but I will double check.

[weavejester]

It might be possible to launch a future from the middleware and use deref with timeout there, but then the async nature and :timeout key in the server setup won't play along, right?

It's true they won't, but by default there is no timeout set. So rather than setting a timeout on the adapter, a more general solution might be to create some middleware that sends a timeout response after a set time, and then ignores all respond and raise calls after that.

I saw that the adapter calls sendError so the error response is generated by Jetty and isn't following the Ring semantics, but I can revert this change.

As a last resort, yes. But custom error reporting can be added in as middleware, so long as the exception stems from the handlers rather than the adapter itself.

Sure thing, I thought I had taken care of this, but I will double check.

Specifically, the line lengths of code should be 80, the subject line of the commit 50, and the commit message body should wrap at 72.

[mourjo]

So rather than setting a timeout on the adapter, a more general solution might be to create some middleware that sends a timeout response after a set time, and then ignores all respond and raise calls after that.

This makes sense but that way it won't consider the time the request spends in Jetty's queue. If I understand correctly, until Jetty dispatches the request, the async handler the adapter is building won't get a chance to execute, hence the timeout it can count for is not the timeout the client will see (which will include the time spent in the request queue). Is there any other way to do this?

As a last resort, yes. But custom error reporting can be added in as middleware, so long as the exception stems from the handlers rather than the adapter itself.

Fair enough, I will remove this error handler change.

Specifically, the line lengths of code should be 80, the subject line of the commit 50, and the commit message body should wrap at 72.

Thanks for explaining this, I will make the change.

[weavejester]

This makes sense but that way it won't consider the time the request spends in Jetty's queue. If I understand correctly, until Jetty dispatches the request, the async handler the adapter is building won't get a chance to execute, hence the timeout it can count for is not the timeout the client will see (which will include the time spent in the request queue).

That's a reasonable point. I think an :async-timeout-handler would be useful in those situations where a precise timeout matters.

[mourjo]

Okay, thanks. Now that I think about it, it would be useful in this context to also capture the time the request came to Jetty, so if we have a timestamp in the request map, the middleware can decide whether to drop the request or not (since the client might already be timed out). Something like this, using this:

(defn- build-request-map
  [^org.eclipse.jetty.server.Request base-request ^HttpServletRequest request]
  (assoc (servlet/build-request-map request)
         :request-ts-millis (.getTimeStamp base-request)))

This alone cannot substitute the timeout on async handlers because if the requests are piling up on Jetty's queue, the client connections will not be cleared until the Jetty handler is called, but this gives a flexibility to drop requests which have waited too long. What do you think of this?

[weavejester]

That's an interesting idea... I'll need time to consider it, as any key added to the request has more lasting repercussions than those added to the Jetty options.

However, my initial thought is to add it to the Ring 2 branch as an optional field called :ring.request/timestamp that maps to a java.time.Instant object.

[mourjo]

Alright, sounds good! I'll make the timeout handler changes here then.

Let me know if you'd like me to create an issue/pull request on the 2.0 branch for the request timestamp tracking.

[mourjo]

I have pushed the changes to just include the timeout handler (with the formatting changes).

[weavejester]

Thanks. I'll keep this PR around, though my inclination is just to add a timestamp to the request map and let middleware handle the timeout, as that makes the timeout code not tied to a specific adapter.

[mourjo]

Okay but in that case, if there are too many requests coming in, client connections won't be cleared until the Jetty handler (including the middleware) gets called.

[weavejester]

Precise timeouts are probably not necessary for many applications, but since we have a timeout option there already, adding a timeout handler seems fine.

However, it shouldn't be a synchonous handler, but an asynchronous one. The tests should also be more robust, in that we shouldn't be matching exact Jetty response HTML. There's also a few formatting and documentation issues in the code provided, but let's fix the functional parts first, and I'll cover all the other issues in the subsequent review.

[mourjo]

However, it shouldn't be a synchonous handler, but an asynchronous one.

Yes the async-handler option is used only for the building the async-proxy-handler (it is not used when the :async? option is not truthy). Am I misunderstanding something?

The tests should also be more robust, in that we shouldn't be matching exact Jetty response HTML

Okay I will rewrite this test case.

[weavejester]

Yes the async-handler option is used only for the building the async-proxy-handler (it is not used when the :async? option is not truthy). Am I misunderstanding something?

It should use the 3-arity form, rather than the 1-arity form.

[weavejester]

The tests should also be more robust, in that we shouldn't be matching exact Jetty response HTML

Okay I will rewrite this test case.

We should still be testing that a timeout is generated even if the handler is not set, however. We just don't need to match the exact HTML, just check the status.

[mourjo]

Alright, pushing a test for the status check.

[mourjo]

It should use the 3-arity form, rather than the 1-arity form.

Correct me if I'm wrong in my understanding here but the way I thought of the timeout handler is this: the asynchronous adapter (currently) expects (a handler which expects) two callbacks: the respond callback and the error callback. These callbacks are not themselves async and therefore expect one argument (the request map or an exception). In that line of thinking, the "timeout handler" is another callback that is passed to the async adapter and it expects one argument like the other two callbacks. Perhaps the term "timeout handler" was misinterpreting the intention of the function.

[weavejester]

The respond and raise callbacks are called by the developer, whereas the timeout handler is called by the adapter.

Calling it a "handler" is correct under Ring parlance, as it is given a request map and is expected to deliver a response. However, making it a synchronous handler is inconsistent with the asynchronous handlers the adapter expects for normal use. In addition, it means we cannot use asynchronous callbacks when handling the timeout.

For these reasons, the timeout handler should be asynchronous, and use the 3-arity form.

[mourjo]

I have made the change for the timeout handler to be an async-style handler.

[mourjo]

Thanks for your comments, I have made the changes

[weavejester]

Thanks! This looks fine. The only thing that needs to be changed is the commit message in order to adhere to the contributing guidelines. Can you change it to:

Add async timeout hander to Jetty adapter

Add a new :async-timeout-handler option to the Jetty adapter to
customize what happens when the :async-timeout is exceeded.

[mourjo]

Done! 🙇

[mourjo]

Thanks for the merge. I wanted to get you opinion on the following: Will you be considering adding the key to Ring's request map for the timestamp for getting the request ingestion time into Jetty? The one referred to here https://github.com/ring-clojure/ring/pull/410#issuecomment-667623112

What my intention here is this: If a request is waiting for long in the queue, find a way to know about it in the handler. https://github.com/mourjo/procrustes/blob/master/src/procrustes/core.clj#L50 Alternatively it could also be possible to use coercion function passed to the Jetty adapter to generate custom keys like these.

[weavejester]

Will you be considering adding the key to Ring's request map for the timestamp for getting the request ingestion time into Jetty?

For Ring 2.0, certainly. I'm not sure about 1.x.

[mourjo]

Sounds good, thanks!