[tor-bugs] #16225 [Metrics/metrics-lib]: Unify exception/error handling in metrics-lib

[Tor Bug Tracker & Wiki]

#16225: Unify exception/error handling in metrics-lib
---------------------------------+-----------------------------------
 Reporter:  karsten              |          Owner:  karsten
     Type:  enhancement          |         Status:  needs_information
 Priority:  Medium               |      Milestone:  metrics-lib 1.7.0
Component:  Metrics/metrics-lib  |        Version:
 Severity:  Normal               |     Resolution:
 Keywords:                       |  Actual Points:
Parent ID:                       |         Points:
 Reviewer:                       |        Sponsor:
---------------------------------+-----------------------------------

Comment (by karsten):

 Replying to [comment:10 iwakeh]:
 > Replying to [comment:8 karsten]:
 > > ...
 > > > From this scenario, I would conclude that methods like
 readDescriptors, parseDescriptors, collectDescriptors don't throw
 DescriptorParseExceptions, but simply process as much as possible and
 provide a list of problematic descriptors at the end, which the API using
 program can choose to ignore or process.
 > > > Does this sound like the right use-case description?
 > >
 > > It does sound like the right use-case description!
 >
 > Maybe, add that to the javadoc somewhere :-)

 Yes, we should, once we fully support that use case.  Right now, if
 `DescriptorParser` is tasked with parsing a given file and runs into
 something it cannot parse, it just throws that exception and calls it a
 day (see #22139).

 >
 > >
 > > The part that needs more thoughts is when and how we should provide
 problematic descriptors and related error cases.  Providing them at the
 end can easily lead to out-of-memory situations, because we might have to
 keep more and more problematic descriptors in memory until we're finally
 done parsing.  Maybe we can include problematic descriptors in line with
 non-problematic ones.
 > >
 > > How about this: whenever we can't parse a descriptor, we include a
 `Descriptor` instance with the raw contents we cannot parse in the
 descriptor queue.  And we include a new method `Descriptor#getException()`
 that returns the `DescriptorParseException` that we ran into, or `null` if
 we didn't run into an exception while parsing.
 > >
 > > Similarly, if we run into an exception while downloading files from a
 remote server or while reading files from the file system, so before
 splitting contents into descriptor-sized chunks and attempting to parse
 those, we include a `Descriptor` instance without descriptor contents and
 with just the exception.
 >
 > If we were using java8, I'd suggest using
 java.util.Optional<Descriptor>.

 It seems like we won't have Java 8 in the near future (see #19622), but
 can you elaborate how that would help here?

 > Actually your suggestions goes in that direction, too.
 > Maybe, a new descriptor class InvalidDescriptor could be useful here?
 It would be accessible via the instanceof-method and have limited
 information: at least the Exception and maybe also its origin (url, path),
 in case of a file also bytes.

 I briefly thought about such a new type for this case, too.  But the only
 reason I found was that there wouldn't be a method
 `Descriptor#getException()` that sometimes returns `null`.  However,
 adding such a type would mean that whoever is interested in
 exceptions/errors would have to do another `instanceof` check and
 downcast.  Are there other aspects?

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/16225#comment:11>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online