[GitHub] [cxf] reta opened a new pull request #748: [WIP] CXF-8422: Unclosed input streams after using org.apache.cxf.tools.wsdlto.WSDLToJava (targeting StAX's XMLStreamReader)

[GitBox]

reta opened a new pull request #748:
URL: https://github.com/apache/cxf/pull/748


   ```
       /**
        *<p>
        * Note: as per StAX 1.0 specs, this method does NOT close the underlying
        * input reader. That is, unless the new StAX2 property
        * {@link org.codehaus.stax2.XMLInputFactory2#P_AUTO_CLOSE_INPUT} is
        * set to true.
        */
       @Override
       public void close() throws XMLStreamException
   ```
   
   But it still unclear why the file handle is not freed up.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

reta commented on pull request #748:
URL: https://github.com/apache/cxf/pull/748#issuecomment-782945121


   @coheigea @amarkevich mind taking a look guys? one more resource leak in certain circumstances in case of `XMLStreamReader` usage


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

amarkevich commented on a change in pull request #748:
URL: https://github.com/apache/cxf/pull/748#discussion_r580143043



##########
File path: core/src/main/java/org/apache/cxf/staxutils/StaxUtils.java
##########
@@ -1660,22 +1667,38 @@ public static XMLStreamReader createXMLStreamReader(InputSource src) {
         String sysId = src.getSystemId() == null ? null : src.getSystemId();
         String pubId = src.getPublicId() == null ? null : src.getPublicId();
         if (src.getByteStream() != null) {
+            final InputStream is = src.getByteStream();
+
             if (src.getEncoding() == null) {
-                StreamSource ss = new StreamSource(src.getByteStream(), sysId);
+                final StreamSource ss = new StreamSource(is, sysId);
                 ss.setPublicId(pubId);
-                return createXMLStreamReader(ss);
+                
+                final XMLStreamReader xmlStreamReader = createXMLStreamReader(ss);
+                if (AUTO_CLOSE_INPUT_SOURCE) {
+                    return new AutoCloseableXMLStreamReader(xmlStreamReader, is);

Review comment:
       AutoCloseable wont close source stream because returning XMLStreamReader cant be defined inside try-with-resources block




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

reta commented on a change in pull request #748:
URL: https://github.com/apache/cxf/pull/748#discussion_r580233404



##########
File path: core/src/main/java/org/apache/cxf/staxutils/StaxUtils.java
##########
@@ -1660,22 +1667,38 @@ public static XMLStreamReader createXMLStreamReader(InputSource src) {
         String sysId = src.getSystemId() == null ? null : src.getSystemId();
         String pubId = src.getPublicId() == null ? null : src.getPublicId();
         if (src.getByteStream() != null) {
+            final InputStream is = src.getByteStream();
+
             if (src.getEncoding() == null) {
-                StreamSource ss = new StreamSource(src.getByteStream(), sysId);
+                final StreamSource ss = new StreamSource(is, sysId);
                 ss.setPublicId(pubId);
-                return createXMLStreamReader(ss);
+                
+                final XMLStreamReader xmlStreamReader = createXMLStreamReader(ss);
+                if (AUTO_CLOSE_INPUT_SOURCE) {
+                    return new AutoCloseableXMLStreamReader(xmlStreamReader, is);

Review comment:
       That is correct (unless typecasted) but the value here is in how `AutoCloseableXMLStreamReader` defines `close` method: closing the reader and input stream. In my initial attempt, the `AutoCloseableXMLStreamReader` was promoted to return value (covering try-with-resources) but the consequences of closing the `InputSource` streams all the time are not clear to me. It could break things easily.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

amarkevich commented on pull request #748:
URL: https://github.com/apache/cxf/pull/748#issuecomment-784219240


   @coheigea yes; probably due to https://github.com/eclipse/jetty.project/issues/4275


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

reta commented on pull request #748:
URL: https://github.com/apache/cxf/pull/748#issuecomment-784237162


   @coheigea @amarkevich too often Jetty minor update is a surprise, I will revert it for now, sorry for that guys


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

reta commented on pull request #748:
URL: https://github.com/apache/cxf/pull/748#issuecomment-784350973


   @amarkevich @coheigea thanks for heads up guys, tests should be back to green


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

reta merged pull request #748:
URL: https://github.com/apache/cxf/pull/748


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]