eclipse-ee4j / jersey

Eclipse Jersey Project - Read our Wiki:
https://github.com/eclipse-ee4j/jersey/wiki
Other
691 stars 356 forks source link

InboundMessageContext.hasEntity() throws IllegalStateException when stream has been closed #5383

Open wilsongee opened 1 year ago

wilsongee commented 1 year ago

Relevant code:

    public boolean hasEntity() {
        entityContent.ensureNotClosed();

        try {
            return entityContent.isBuffered() || !entityContent.isEmpty();
        } catch (IllegalStateException ex) {
            // input stream has been closed.
            return false;
        }
    }

It appears that we explicitly check to see if the stream is closed here: https://github.com/eclipse-ee4j/jersey/blob/2.x/core-common/src/main/java/org/glassfish/jersey/message/internal/InboundMessageContext.java#L814

It looks like we handle the exception and return false here: https://github.com/eclipse-ee4j/jersey/blob/2.x/core-common/src/main/java/org/glassfish/jersey/message/internal/InboundMessageContext.java#L820

Stacktrace with personal lines scrubbed out:

Entity input stream has already been closed. 
    org.glassfish.jersey.message.internal.EntityInputStream.ensureNotClosed(EntityInputStream.java:205)
    org.glassfish.jersey.message.internal.InboundMessageContext.hasEntity(InboundMessageContext.java:780)
    ...

In this particular case, I think it makes sense to move the entityContent.ensureNotClosed(); statement into the try block below it.

To work around this issue, I have to handle my invocation of hasEntity() myself outside of an if-statement for legibility sake.

Sample work around:

    boolean hasEntity = false;
    try {
        hasEntity = requestContext.hasEntity();
    } catch (IllegalStateException e) {
    }

    if (hasEntity) {
        ...
    }
jansupol commented 1 year ago

I believe the exception is thrown on purpose - at the time the stream is closed, the attempts for reading the entity are unexpected.

From the issue description, the use case for reading the entity when the stream is closed is not clear. In what situation do you read from the closed stream?

wilsongee commented 1 year ago

entityContent.isEmpty() calls ensureNotClosed() in its first line and is then caught when there is a possible IllegalStateException. That means the author of this code fragment intended to catch the IllegalStateException and return false.

It isn't because we're actively trying to read when the stream was closed by us. I believe there might be a race condition in our code somewhere and there could be a small chance, as evident by the handful of errors in our Splunk logging, that it could happen.