Bartosz Baranowski <bbaranow> updated the status of jira RESTEASY-1182 to Coding In Progress

Ron, are you OK with finalize() block approach ?

if this is deemed a 'no good' I can probably hack that scanner, but it does not seem a good idea, it just adds configuration complexity.

Hi Bartosz,

I'm sorry I forgot about this bz. I think I saw finalize() and had a Monty Python moment: "Run away, run away". 

There are two resources that should be eliminated:

 1. any system resources associated with the InputStream, handled by InputStream.close(), and

 2. the temporary file.

We leave the responsibility for calling close() to the user, and failing to call close() would be considered a bug. So why not do something like this:

     public InputStream getInputStream() throws IOException
      {
         InputStream bis = new ByteArrayInputStream(byteBuffer, byteBufferOffset, byteBufferLength);
         if (tempFile == null)
            return bis;
         InputStream fis = new FileInputStream(tempFile);
         return new SequenceInputStream(bis, fis) {
            @Override
            public void close() throws IOException
            {
               super.close();
               deleteTempFile();
            }
         };
      }

1. We are handing the responsibility for removing the temp file back to the user in a way that seems reasonable.

2. We could still have the finalize() method as a back up.

3. Having said all that, I think it's a good thing to clean up the temp file, but I'm not sure how terrible it is to not clean it up, given that it's going into a temporary file directory.

-Ron

Well, problem here is that getInputStream must return new instance of IS that is positioned at the beginning of the data:
{quote}
This method returns an InputStream representing the data and throws the appropriate exception if it can not do so. Note that a new InputStream object must be returned each time this method is called, and the stream must be positioned at the beginning of the data.
{quote}

So the trick with IS.close+deleteTMP file will work as long as users make sure:
 - they will call it eventually
 - call happens 


Trick with that is if:
 - call happens to soon, we violate specs
 - call does not happen, we have a leak


So with this one, its lesser evil. Either we violate specs or have leak which either we can:
 - leave
 - use deleteOnExit() and kill server/JVM

None of those is a good choice.

Hi Bartosz,

You're right. Nothing I said made any sense.

I'm thinking a reasonable approach is to tie these temporary files to the servlet request lifecycle, i.e., solution 3) in the list given above. I'll have to look at the code to see just how to do it, but maybe it's not too complicated.

-Ron

I'm playing with defining a Cleanable interface, a Cleanables class with a set of Cleanable's, adding Cleanables.class to the context map, defining an extension of SequenceInputStream that implements Cleanable, etc, etc. SynchronousDispatcher.pushContextObjects() puts Cleanables in the map and SynchronousDispacher.clearContextData() gets all the Cleanables and calls clean() on all of them. DataSourceProvider adds the CleanableSequenceInputStream to the Cleanables object in the context map. CleanableSequenceInputStream.clean() calls deleteTempFile().

So far, it's working, but I want to do more testing.

Ha, did not know this was possible here. Seems best choice so far.

Ron Sigal <ron.sigal> updated the status of jira RESTEASY-1182 to Resolved

Ron Sigal <ron.sigal> updated the status of jira RESTEASY-1182 to Closed

Verified in EAP 6.4.3.CP.CR2.

Katerina Novotna <kanovotn> updated the status of jira RESTEASY-1182 to Reopened

Weinan Li <weli> updated the status of jira RESTEASY-1182 to Resolved

Alessio Soldano <asoldano> updated the status of jira RESTEASY-1182 to Closed

Retroactively bulk-closing issues from released EAP 6.4 cummulative patches.

Retroactively bulk-closing issues from released EAP 6.4 cumulative patches.

Retroactively bulk-closing issues from released EAP 6.4 cumulative patches.