Wording clarification in ContainerRequestContext#setEntityStream

[ben-manes]

When implementing a ContainerRequestFilter that replaced the EntityStream, I noticed that Rest-Easy's implementation did not close it automatically. The method says The runtime is responsible for closing the input stream which indicated to me that the implementation should close the stream when the request completes. The issue response was to ask here for clarification.

The original intent was to validate the Content-Length header for file uploads to reject a partial file transfer (e.g. mobile upload). This required consuming the entity stream to a temporary file, verifying the length, and replacing the entity stream with one backed by this file for downstream usage. This worked fine, but when testing the stream's close method was not called. A minimal test verifies this where the finalizer prints Was closed? false.

What is the expectation when setEntityStream is used and who is responsible for closing it?

@javax.ws.rs.ext.Provider
public final class TestInterceptor implements ContainerRequestFilter {

  @Override
  public void filter(ContainerRequestContext requestContext) {
    requestContext.setEntityStream(new IsClosedInputStream(requestContext.getEntityStream()));
  }

  private static final class IsClosedInputStream extends FilterInputStream {
    private volatile boolean isClosed;

    protected IsClosedInputStream(InputStream source) {
      super(source);
    }

    @Override
    public void close() throws IOException {
      isClosed = true;
      super.close();
    }

    @Override
    protected void finalize() {
      System.err.println("Was closed? " + isClosed);
    }
  }
}

[andymc12]

My opinion is that the JAX-RS implementation should close the input stream - so in your example, I would expect to see Was closed? true in the output.

That said, there probably needs to be some clarification here - on when and which streams the JAX-RS implementation is required to close. For example:

public void filter(ContainerRequestContext requestContext) {
    InputStream is1 = reqCtx.getEntityStream();
    InputStream is2 = new FilterInputStream(is1);
    reqCtx.setEntityStream(is2); // should JAX-RS close is1 here? that would break things for is2...
        // should it close is1 later?
    InputStream is3 = new FileInputStream("/someFile");
    reqCtx.setEntityStream(is3); // should JAX-RS close is2? 
    InputStream is4 = new BufferedInputStream(is2);
    reqCtx.setEntityStream(is4);
}

public void filter(ContainerRequestContext reqCtx, ContainerResponseContext resCtx) {
    // what if we want to read the input stream after the resource method was invoked?
    InputStream is4 = reqCtx.getEntityStream(); // should be the same as is4 above assuming nobody else changes it...
    is4.reset(); // so we can log the full request and response?
    //...
}

This is not a common scenario but is JAX-RS responsible for closing all four input streams? And when should they be closed? If we close the old stream when a new one is set, we might end up closing a stream that was intended to be delegated to... If we close after the resource method is invoked, we might prevent the response filters from reading the stream...

I lean toward the idea that if a user sets a new entity stream, then they are responsible for closing the original stream. And I think that the last set entity stream should be closed by JAX-RS after all response filters have executed. In the example above, JAX-RS would close is4 (which in turn would close is2 and is1 via delegation) - and the user would have "leaked" is3.

This is just my two cents - but I don't think it is quite clear what JAX-RS implementations are supposed to do. That said, it does seem like a bug to me that RESTEasy is not closing the stream.

[ben-manes]

Thanks. I think my expectations are the same as yours. I would find it reasonable to, a) JAX-RS closes the original stream b) JAX-RS closes the last stream set c) The user must close any other intermediate streams by chaining on their stream's close method

In the RestEasy behavior, I think the user would have to capture the streams and close on a ContainerResponseFilter. That could be hacky, e.g. needing to use a ThreadLocal which may is a rigid assumption. For my particular case, I ended up using a Jetty Handler (and can likely remove it if confirmed to be a non-server bug, as I think partial transfers would be discovered and rejected during the multipart body parsing).