[
https://issues.apache.org/jira/browse/IO-192?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12672616#action_12672616
]
Jukka Zitting commented on IO-192:
----------------------------------
I like how you're extending the functionality to the Reader and Writer classes.
{quote}
1) Its a useful feature to be able to handle exceptions - not just in this
use-case for tagging, but generally so IMO it would be good to move the
exception handling into the Proxy stream implementations. We could provide a
protected handleException(IOException) method that by default just re-throws
the exception to keep compatibility, but a allows people to override for their
own custom exception handling.
{quote}
Good idea.
My only issue is with the return value in the handleException() method of
ProxyInputStream and ProxyReader. For example the skip() method should never
return -1 but there is no way (apart from parsing the stack trace) for
handleException() to know which method invoked it and what return value would
be appropriate. I'd rather have the handleException() method return nothing,
and just add fixed "return -1" or "return 0" statements where needed. A
subclass that needs to modify the return value based on a thrown exception
should override the specific method with custom processing.
{quote}
2) Exceptions are Serializable and many stream implementations are not so I
have some concern about holding a reference to the stream in the
TaggedIOException. Also this could cause references to the stream being held
longer than previously by the application and prevent/delay garbage collection.
An alternative could be to store the identity hash code of the tag object
instead.
{quote}
Good point, though I'm not so sure about using the identity hash for this. For
most (all?) JVMs it will be unique to the tag object (at least as long as the
object lives), but there's no guarantee that this actually is the case. Perhaps
the tagged proxy classes should have a "private final Object tag = new
Object();" tag object for this purpose. This would make the related IOUtils
methods more complex, but see below for more on that.
{quote}
3) The current solution requires users to reference the concrete tagged stream
implementations. While this is OK in your simple example within a single method
its not good practice generally and will either encourage people to pollute
their API with these tagged streams or require additional casting.
{quote}
I don't see a use case where you'd need to use casts or pollute APIs with these
classes.
{quote}
I suggest we move the code for handling these streams into IOUtils - which also
makes it more generic and available to re-use for other tagging requirements,
not just by the throwing stream.
{quote}
I would rather put such static generic methods directly on the
TaggedIOException class. This would make it easier to reuse just that class.
In any case I would keep the current isCauseOf() and throwIfCauseOf() methods
on the tagged stream classes, as IMHO the instance method call is clearer than
a static IOUtils (or TaggedIOException) method call.
> Tagged input and output streams
> -------------------------------
>
> Key: IO-192
> URL: https://issues.apache.org/jira/browse/IO-192
> Project: Commons IO
> Issue Type: New Feature
> Components: Streams/Writers
> Reporter: Jukka Zitting
> Assignee: Jukka Zitting
> Priority: Minor
> Fix For: 1.5
>
> Attachments: IO-192-tagged-stream-changes.patch, IO-192.patch
>
>
> I'd like to introduce two new proxy streams, TaggedInputStream and
> TaggedOutputStream, that tag all exceptions thrown by the proxied streams.
> The goal is to make it easier to detect the source of an IOException when
> you're dealing with multiple different streams. For example:
> {code}
> InputStream input = ...;
> OutputStream output = ...;
> TaggedOutputStream proxy = new TaggedOutputStream(output);
> try {
> IOUtils.copy(input, proxy);
> } catch (IOException e) {
> if (proxy.isTagged(e)) {
> // Could not write to the output stream
> // Perhaps we can handle that error somehow (retry, cancel?)
> e.initCause(); // gives the original exception from the proxied stream
> } else {
> // Could not read from the input stream, nothing we can do
> throw e;
> }
> }
> {code}
> I'm working on a patch to implement such a feature.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
