[jira] [Comment Edited] (WICKET-4600) Remove IResourceStream.close()

["Andrea Del Bene (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/WICKET-4600?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14149277#comment-14149277 ] 

Andrea Del Bene edited comment on WICKET-4600 at 9/26/14 3:12 PM:
------------------------------------------------------------------

counter-order :). I've investigated the code a little more and purpose of close() is actually confusing. At the moment we have 6 classes that implements this method:

AbstractResourceStreamWriter
AbstractStringResourceStream
FileResourceStream
ResourceStreamWrapper
XSLTResourceStream
ZipResourceStream 

4 of them provide just an empty implementation (i.e. method body is empty). The only class that makes something meaninful inside close() is FileResourceStream and the reason is that it tries to reuse the same inputstream if we don't explicitly invoke close. This beahvior doesn't look so useful to me, as we usually need a brand  new input stream when we need to read a file.
Having said that, I would also add that client code is not required to know that an inputStream comes from an IResourceStream, hence it may append that InputStream#close is invoked while IResourceStream#closed is ignored (see for example AbstractMarkupParser as described in  WICKET-5700).  


was (Author: bitstorm):
counter-order :). I've investigated the code a little more and purpose of close() is actually confusing. At the moment we have 6 classes that implements this method:

AbstractResourceStreamWriter
AbstractStringResourceStream
FileResourceStream
ResourceStreamWrapper
XSLTResourceStream
ZipResourceStream 

4 of them provide and empty implementation (i.e. method body is empty). The only class that makes something meaninful inside close() is FileResourceStream and the reason is that it tries to reuse the same inputstream if we don't explicitly invoke close. This beahvior doesn't look so useful to me, as we usually need a brand  new input stream when we need to read a file.
Having said that, I would also add that client code is not required to know that an inputStream comes from an IResourceStream, hence it may append that InputStream#close is invoked while IResourceStream#closed is ignored (see for example AbstractMarkupParser as described in  WICKET-5700).  

> Remove IResourceStream.close()
> ------------------------------
>
>                 Key: WICKET-4600
>                 URL: https://issues.apache.org/jira/browse/WICKET-4600
>             Project: Wicket
>          Issue Type: Improvement
>          Components: wicket
>    Affects Versions: 6.0.0-beta2
>            Reporter: Jesse Long
>
> The IResourceStream.close() method is designed to allow the IResourceStream implementations to destroy data over and above the InputStream.
> If it was just the InputStream, we would not need the method, we could just call close() on the InputStream.
> The problem is, almost none of the wicket code correctly calls IResourceStream.close(). Often, IResourceStream.getInputStream() is called, often the InputStream is then closed, but IResourceStream.close() is not called.
> Also, it is not clear at which point IResourceStream.close() needs to be called. Possibly only after an InputStream is created, in which case, why does InputStream.close() not suffice? Otherwise, we should call IResourceStream.close() every time we use it - this very certainly does not happen.
> I propose to remove this method to save wicket the effort. I really doubt there is much real need to clean anything up in the close() method. IResourceStream implementation should retain enough information  to create an InputStream, and create an InputStream method in the getInputStream() method, and close any no-longer needed resources in the getInputStream() method. Consumers of the IResourceStream can just close the InputStream and be done with it.
> Here is some discussion about some of the dodgey uses of IResourceStream.close in wicket ATM:
> AbstractMarkupParser calls IResourceInputStream.getInputStream(), but never calls IResourceStream.close().
> ContextRelativeResource call IResourceStream.getInputStream(), but never calls IResourceStream.close().
> ResourceStreamResource calls close() on the IResourceResponse returned from internalGetResourceStream(). However, it is possible that a new IResourceStream is dynamically created each time internalGetResourceStream is called, in which case ResourceStreamResponse closes the incorrect IResourceStream. Live would be easier if it only had to close the InputStream.
> MessageDigestResourceVersion calls IResourceStream.getInputStream(), but never calls IResourceStream.close().
> ConcatBundleResource calls IResourceStream.getInputStream(), but never calls IResourceStream.close(). (ConcatBundleResource also does stupid things with ByteArrayInputStream btw).
> XSLTResourceStream calls (in its constructor) IResourceStream.getInputStream() but never calls IResourceStream.close(). XSLTResourceStream and its ZipDirectoryResourceStream friend really want to be deleted.
> Also, there are many many usages of IResourceStream where close() is not called, but getInputStream() is not called either.
> Wont mind working on a patch, after some direction from wicket devs.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)