Am I closing my input stream correctly in Java?

Question

I created a class that extends InputStream so that I can keep count of the number of bytes being read and throw an exception if it exceeds a max limit that I define.

Here is my class:

    public class LimitedSizeInputStream extends InputStream
    {

        private final InputStream original;
        private final long maxSize;
        private long total;

        public LimitedSizeInputStream(InputStream original, long maxSize)
        {
            this.original = original;
            this.maxSize = maxSize;
        }

        @Override
        public int read() throws IOException
        {
            int i = original.read();
            if (i >= 0)
            {
                incrementCounter(1);
            }
            return i;
        }

        @Override
        public int read(byte b[]) throws IOException
        {
            return read(b, 0, b.length);
        }

        @Override
        public int read(byte b[], int off, int len) throws IOException
        {
            int i = original.read(b, off, len);
            if (i >= 0)
            {
                incrementCounter(i);
            }
            return i;
        }

        private void incrementCounter(int size) throws IOException
        {
            total += size;
            if (total > maxSize)
            {
                throw new IOException("InputStream exceeded maximum size in bytes.");
            }
        }
    }

This is coming from: Copy InputStream, abort operation if size exceeds limit, I am implementing a Jersey API that needs to fail if a user is uploading a file that is too large.

Here is my resource class:

    @POST
    @Consumes(MediaType.MULTIPART_FORM_DATA)
    @Path("/test")
    public Response load(
        @Context HttpServletRequest request,
        @FormDataParam(FILE_FIELD) FormDataBodyPart file)
    {
      if (request.getContentLength() > MAX_FILE_SIZE_IN_BYTES)
        {
            // fail fast handle failure
        }

      try (InputStream stream = new LimitedSizeInputStream(
           file.getValueAs(InputStream.class), MAX_FILE_SIZE_IN_BYTES))
        {
            // some logic
        }
      catch (IOException e)
        {
            // handle failure
        }
}

I wrapped LimitedSizeInputStream in my try resource so I think the stream should close properly. I'm just a bit confused as to whether the close is handled correctly or if I'm technically opening two input streams through LimitedSizeInputStream and file.getValueAs(InputStream.class) and only one is closing?

It looks like you implemented something similar to apache commons BoundedInputStream. You might look at their implementation for guidance at least. — Deadron, May 11 '20 at 20:36
@Deadron similar but not the same. BoundedInputStream doesn't throw an error it just stops reading further — ronapple14, May 12 '20 at 01:27
You should have extended `FilterInputStream`. Don't repost questions here. — user207421, May 12 '20 at 05:39

areus · Answer 1 · 2020-05-12T05:19:39.450

3

The try-with-resources only closes the declared resource. So will only close metadataStream.

You should implement the close method in LimitedSizeInputStream to close the original stream.

@Override
public void close() throws IOException {
    original.close();
}

edited May 12 '20 at 05:19

answered May 11 '20 at 20:35

areus

2,880
2
7
17

oh ok... that's not good. is there an elegant way to close both without making the code messy? – ronapple14 May 12 '20 at 01:28
1

I added the required code. That's not messy. If you use an inner input stream, to delegate read calls, it's normal to also delegate the close call. – areus May 12 '20 at 05:22
but won't that change the behavior so only the inner input stream is closed now and not the LimitedSizeInputStream? – ronapple14 May 12 '20 at 05:48
1

The purpose of the `close` method is to release any system resources. The only resource that `LimitedSizeInputStream` uses is the underlying original `InputStream` – areus May 12 '20 at 08:08

score 1 · Answer 2 · answered May 12 '20 at 05:37

1

If LimitedSizeInputStream extends InputStream and wraps another stream, then @areus's solution is the best one.

An alternative approach would be to extend FilterInputStream, like this:

public class LimitedSizeInputStream extends FilterInputStream
{
    private final long maxSize;
    private long total;

    public LimitedSizeInputStream(InputStream original, long maxSize)
    {
        super(original);
        this.original = original;
        this.maxSize = maxSize;
    }

    // use 'this.in' instead of 'this.original'
    // at least one of the 'read' methods needs to be overridden.
}

Note that FilterInputStream provides default implementations of the API methods that may prove useful.

The javadoc provides details of what the default method implementations do.

answered May 12 '20 at 05:37

Stephen C

698,415
94
811
1,216

If I override close method and only close the inner stream, how does the LimitedSizeInputStream close? – ronapple14 May 12 '20 at 05:50
You don't need to override close if you use this approach. The inherited `FilterInputStream:close` method closes `in`. That is the only thing that actually needs to be closed. – Stephen C May 12 '20 at 06:15
Another way of looking at this is that your override `close()` closes the `LimitedSizeInputStream`. The purpose of `close` is to release any resources. The only resource that belongs to your custom stream is the `original` stream that it wraps. It makes no difference if you close it directly, or get the superclass to close it by delegating via `super.close()`. – Stephen C May 12 '20 at 06:20

Am I closing my input stream correctly in Java?

2 Answers2

Linked

Related