helidon icon indicating copy to clipboard operation
helidon copied to clipboard

4.x: Response.beforeSend(Runnable)

Open romain-grecourt opened this issue 1 year ago • 1 comments

Environment Details

  • Helidon Version: 4.x
  • Helidon SE

Problem Description

Prior to 4.x, SE had provided a way to augment the headers before sending them with response.headers().beforeSend():

(req, res) -> {
  res.headers().beforeSend(headers -> {
    // augment response headers right before sending them
  });
}

See javadoc here.

This method has been removed in 4.x.


I'm assuming that the reasoning is that since 4.x is fully blocking, mutating headers before sending the response is as simple as adding a statement before response.send().

(req, res) -> {
  res.header("foo", "bar");
  res.send();
}

The code above wouldn't be accurate in 3.x and prior thus the existence of ResponseHeaders.beforeSend.

In 4.x, we don't need the beforeSend as one can naturally do that in a terminating handler like shown above.

However, we loose the ability to register a function that can mutate the response “before send” from a filter (a non terminating handler).


Here is an ugly workaround that uses Http1Config to register a "receive" listener to create a new future in the request context, and a "send" listener to complete the future when headers are passed.

WebServer.builder()
        .port(8080)
        .routing(rules -> rules
                .any((req, res) -> {
                    // a filter that adds a response header with the total time for the request
                    long startTime = System.nanoTime();
                    req.context().get("beforeSend", CompletionStage.class)
                            .orElseThrow()
                            .thenRun(() -> {
                                long endTime = System.nanoTime();
                                long totalTime = Duration.ofNanos(endTime - startTime).toMillis();
                                res.header("X-Total-Time", String.valueOf(totalTime));
                            });
                    res.next();
                }).get("/", (req, res) -> {
                    Thread.sleep(2000);
                    res.send("OK");
                }))
        .addProtocol(Http1Config.builder()
                             .addReceiveListener(new Http1ConnectionListener() {
                                 @Override
                                 public void prologue(ConnectionContext ctx, HttpPrologue prologue) {
                                     ctx.listenerContext().context()
                                             .register("beforeSend", new CompletableFuture<Void>());
                                 }
                             })
                             .addSendListener(new Http1ConnectionListener() {
                                 @Override
                                 @SuppressWarnings("unchecked")
                                 public void headers(ConnectionContext ctx, Headers headers) {
                                     ctx.listenerContext().context()
                                             .get("beforeSend", CompletableFuture.class)
                                             .orElseThrow()
                                             .complete(null);
                                 }
                             })
                             .build())
        .build()
        .start();

We should consider adding back a way to react before the response is sent for a filter. E.g.

Response.beforeSend(Runnable)

In fact this mirrors Response.whenSent(Runnable) so that seems reasonable.

romain-grecourt avatar Jan 19 '24 00:01 romain-grecourt

I have an objection to the use case mentioned here: res.header("X-Total-Time", String.valueOf(totalTime)); this will not do what it seems to be doing, as you only compute time it took to start writing a response - if you are for example reading result set from a database, this would happen before the first record is written, long before your route is finished. So you would never write total time, just "time it took the route to attempt to write the response stream" - which is not a very exact measure.

So the only use case I see is that we want to set a header/state based on headers and state configured by the route. There are a few issues that need to be taken into account if this gets implemented:

  • what should happen if an exception is thrown from the runnable
  • what if it changes headers that are entity based (Content-Type, Content-Length)
  • what if it attempts to call send on the response, or requests an input stream?

tomas-langer avatar Feb 08 '24 00:02 tomas-langer