Spring HTTP converters shouldn't enforce skipping close InputStream on server side

It make sense that converters skip closing InputStream on client side to fix https://github.com/spring-projects/spring-framework/issues/27969, but on server side I suggest that converters shouldn't wrap InputStream StreamUtils::nonClosing, the change breaks existing behavior and maybe result in resource leak.

I've encountered a regression since v5.3.16 which introduced by https://github.com/spring-projects/spring-framework/commit/8d5a6520ce033c38c57a9ec026102e17bd598acc .

I'm wrapping HttpServletRequest overriding getInputStream() to return customized ContentCachingInputStream, which will caching request body and log it when close() is called. before that commit, jackson will close the underlying stream since AUTO_CLOSE_SOURCE default true.

    class ContentCachingInputStream extends ServletInputStream {

        private final Logger logger;

        private final ServletInputStream is;

        private final String characterEncoding;

        private FastByteArrayOutputStream cachedContent;

        public ContentCachingInputStream(ServletRequest request, Logger logger) throws IOException {
            this.logger = logger;
            this.is = request.getInputStream();
            this.characterEncoding = request.getCharacterEncoding();
            int contentLength = request.getContentLength();
            this.cachedContent = new FastByteArrayOutputStream(contentLength >= 0 ? contentLength : 1024);

        }

        @Override
        public int readLine(byte[] b, final int off, final int len) throws IOException {
            int count = is.readLine(b, off, len);
            cache(b, off, count);
            return count;
        }

        @Override
        public int read(byte[] b, final int off, final int len) throws IOException {
            int count = is.read(b, off, len);
            cache(b, off, count);
            return count;
        }

        private void cache(byte[] b, final int off, final int count) throws IOException {
            if (count != -1)
                cachedContent.write(b, off, count);
        }

        @Override
        public int read() throws IOException {
            int ch = this.is.read();
            if (ch != -1)
                cachedContent.write(ch);
            return ch;
        }

        @Override
        public boolean isFinished() {
            return this.is.isFinished();
        }

        @Override
        public boolean isReady() {
            return this.is.isReady();
        }

        @Override
        public void setReadListener(ReadListener readListener) {
            this.is.setReadListener(readListener);
        }

        @Override
        public void close() throws IOException {
            try {
                this.is.close();
            } finally {
                if (cachedContent != null) {
                    byte[] bytes = cachedContent.toByteArray();
                    cachedContent = null;
                    String str = new String(bytes, 0, bytes.length, characterEncoding);
                    logger.info("\n{}", str);
                }
            }
        }
    }

Workaround: overrding MappingJackson2HttpMessageConverter to force close after read.

        MappingJackson2HttpMessageConverter jackson2 = new MappingJackson2HttpMessageConverter() {

            @Override
            public Object read(Type type, Class<?> contextClass, HttpInputMessage inputMessage) throws IOException {
                try {
                    return super.read(type, contextClass, inputMessage);
                } finally {
                    inputMessage.getBody().close();
                }
            }

            @Override
            public Object readInternal(Class<?> clazz, HttpInputMessage inputMessage) throws IOException {
                try {
                    return super.readInternal(clazz, inputMessage);
                } finally {
                    inputMessage.getBody().close();
                }
            }
        };

Comment From: rstoyanchev

As you can see from https://github.com/spring-projects/spring-framework/issues/27969#issuecomment-1026711963, this is by design, and while it wasn't consistently enforced, it was the intent.