Ability to associate attributes with HttpMessages/HttpObjects

[rkapsi]

It'd be great if it was possible to associate attributes with HttpMessages (possibly HttpObjects). and effectively make it very easy to implement things such as HTTP request scopes.

I'm currently using a ChannelHandler that creates + fires a "context" user event object for each newly read HttpMessage but that approach has some maintainability problems.

[Scottmitch]

@rkapsi - We love pull requests :) Although if you plan on adding new interfaces to existing interfaces I think this would have to be targeted at 4.1+.

[normanmaurer]

@rkapsi @Scottmitch honestly I think this should be not part of the Http* pojos in the core. If you really want to do this you can just write your own implementation of the interface and do it there.

[rkapsi]

That's a lot to implement though. It'd probably end up being a bunch of wrapper classes that delegate to pojos that HttpObjectDecoder created. I get cold shivers from that.

One thing I've been thinking is if HttpHeaders wouldn't call toString() on header values. It'd be quite elegant and natural for passing values around.

[rkapsi]

... But that would be hacky. I'm personally leaning towards something analogous to HttpServletRequest#setAttribute(...).

[rkapsi]

@Scottmitch @normanmaurer I'm happy to iterate on it. We use Netty with non-aggregated HttpMessages and with many ChannelHandlers. One thing that is consistently missing is the ability to share information on a per-request level.

[Scottmitch]

@normanmaurer - I think the difficulty with creating your own implementation of the interfaces is the codec will not use these objects and as @rkapsi said you must wrap everywhere. Seems like a common problem to have to associate user data with requests/responses? For example in HTTP/2 we have a per stream map where users can store objects.

I see value in this but I don't fully understand @normanmaurer's hesitation... @rkapsi - maybe wait to invest effort implementing this until @normanmaurer expands on his concerns a bit more (FYI he is currently on vacation).

[rkapsi]

@Scottmitch @normanmaurer so what are your thoughts about this? The wrapping is my least favorite option. It's inefficient and would lead to an awkward parallel type system.

One idea could be to change HttpRequestDecoder and its cousins to use something like a customizable HttpRequestFactory. The HttpRequestDecoder already has a factory method for HttpRequest and we'd need something similar for HttpContent and LastHttpContent. It's then up to the user to implement whatever they like and extending from the default implementations is not as bad as having to wrap.

[rkapsi]

Nah, that's not going to work very well with HttpObjectAggregator.

[normanmaurer]

@rkapsi @Scottmitch the concern I have with adding the possibility to expose something like an attribute map in the "pojos" is that this has not much to do with the protocol itself. IMHO the pojos that represent the messages should be very light-weight and not carry any extra stuff that is not part of the protocol itself.

@trustin WDYT?

[rkapsi]

I'd argue "request scopes" and "session scopes" are higher level aspects of the HTTP protocol and my primary (only) motivation for this. Right now there's only "connection scope" (ChannelHandlerContext and Channel) which isn't very useful in the context of HTTP.

In my case, the downstream "client" is a Proxy/Load Balancer that uses connection pooling. The #remoteAddress() is meaningless and I need to pull the client's IP out of the X-Forwarded-For header and store it somewhere. But there is no such place...

This is just one little and arguably lightweight example but there are things that need to be pulled out of a DB and it's very favorable to cache them in the context of the request as it's passing through the pipeline.

Here's an another idea. A new interface that HttpObject implements (similar to DecoderResultProvider) and a new channel handler that is to be installed after the HttpRequestDecoder.

@Sharable
public class HttpPropertiesHandler extends ChannelInboundHandlerAdapter {

  private static final AttributeKey<AttributeMap> PROPERTIES = AttributeKey.valueOf("HttpPropertiesHandler.PROPERTIES");

  public static class MyExampleInitializer extends ChannelInitializer<Channel> {

    @Override
    protected void initChannel(Channel ch) throws Exception {
      ChannelPipeline pipeline = ch.pipeline();

      pipeline.addLast(new HttpServerCodec());
      pipeline.addLast(new HttpPropertiesHandler());
    }
  }

  /**
   * To be implemented by {@link HttpObject}
   */
  public static interface HttpProperties extends AttributeMap {
  }

  /**
   * To be implemented by {@link DefaultHttpObject}
   */
  public static interface DefaultHttpProperties extends HttpProperties {
    public void properties(AttributeMap properties);
  }

  @Override
  public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {

    if (msg instanceof DefaultHttpProperties) {

      AttributeMap properties = null;

      if (msg instanceof HttpMessage) {
        properties = new DefaultAttributeMap();

        if (!(msg instanceof LastHttpContent)) {
          ctx.attr(PROPERTIES).set(properties);
        }

      } else if (!(msg instanceof LastHttpContent)) {
        properties = ctx.attr(PROPERTIES).get();

      } else {
        properties = ctx.attr(PROPERTIES).getAndRemove();
      }

      if (properties != null) {

        if (msg == EMPTY_LAST_CONTENT) {
          msg = new LastHttpPropertiesContent(properties);
        }

        ((DefaultHttpProperties)msg).properties(properties);
      }
    }

    ctx.fireChannelRead(msg);
  }

  private static class LastHttpPropertiesContent implements LastHttpContent, HttpProperties {

    private final AttributeMap properties;

    public LastHttpPropertiesContent(AttributeMap properties) {
      this.properties = properties;
    }

    @Override
    public <T> Attribute<T> attr(AttributeKey<T> key) {
      return properties.attr(key);
    }

    @Override
    public <T> boolean hasAttr(AttributeKey<T> key) {
      return properties.hasAttr(key);
    }

  // ...
}

[trustin]

HttpRequest/ResponseDecoder already has an overridable create*Message() which allows you to use a custom message type. The problem is that HttpObjectAggregator does not allow @rkapsi to use a custom message type. We could add some protected method with default implementation and then all should be well?

[rkapsi]

@trustin The challenge is that a request's scope spans from HttpRequest, HttpContent to LastHttpContent. My suggestion would be to add create methods for the latter two objects as well and possibly even pull that stuff into a factory if braking API isn't a problem.

As for the HttpObjectAggregator, yes that would work.

[ifesdjeen]

Looks like related PR is closed, so that can be also closed by now.