[tor-bugs] #19259 [Metrics/Onionoo]: uncaught NFE

[Tor Bug Tracker & Wiki]

#19259: uncaught NFE
-----------------------------+-----------------------------------
 Reporter:  iwakeh           |          Owner:  iwakeh
     Type:  defect           |         Status:  needs_information
 Priority:  High             |      Milestone:
Component:  Metrics/Onionoo  |        Version:
 Severity:  Major            |     Resolution:
 Keywords:                   |  Actual Points:
Parent ID:                   |         Points:
 Reviewer:                   |        Sponsor:
-----------------------------+-----------------------------------

Comment (by karsten):

 Replying to [comment:7 iwakeh]:
 > OK, I thought you could have a specification (maybe, on paper ;-)
 somewhere, that's why I asked  :-)

 Ah, no, only in my head.

 > Now, I basically just looked at the code and tried to infer what was
 intended.
 > The resulting test draft is added for review.

 I noticed that some of those tests fail.  Mind if I respond to your
 questions below and you figure out whether the code or the test is wrong?

 > This is just concerned with `WeightsStatus`; locale issue follows.
 >
 > Here some findings:
 >
 > 1. First, weights are only counted as missing if less than -.5:
 > {{{
 >         if (weights[i] < -0.5) {
 >           missingValues += 1 << i;
 >         }
 > }}}
 > See also `testMissingValues`.
 > This should be moved into a separate method to make it testable.
 > Does it really do what is intended?

 `WeightsStatus` is mostly an internal status class, and it seemed okay to
 use `-1.0` to encode "missing value".  The check for `< -0.5` was to
 picked to avoid any trouble with double value comparisons, like `< 0.0` or
 `== -1.0`.  Under normal circumstances, values would never have any other
 negative value than `-1.0`.  I'd say let's test this as is and then
 consider changing `weights` to `Double[]` to be able to store `null`
 values.

 The `missingValues += 1 << i` part makes sure that we only combine two
 lines with the exact same pattern of missing values.  We could have used a
 BitSet or anything else, but using bits in an `int` seemed okay here, also
 with regard to performance and avoiding the overhead of using an object
 there.  Documentation would not have hurt, of course. ;)  I'm not saying
 that this is perfect, but at least I believe the code does what it's
 intended to do.

 > 2. There are design choices that could cause trouble:
 >
 > The `setHistory`  method could introduce a history set with a totally
 different comparator. As this method is not used anywhere, it can just be
 removed.  Or, if it should be kept, just take the given history set and
 add the elements to the history after clearing it.  But, better remove
 unused code.

 Huh, I didn't know it's unused.  Yes, unused code should just go away.

 > The second more intricate issue is the Comparator itself:
 >
 > {{{
 >   private SortedMap<long[], double[]> history =
 >       new TreeMap<long[], double[]>(new Comparator<long[]>() {
 >     public int compare(long[] a, long[] b) {
 >       return a[0] < b[0] ? -1 : a[0] > b[0] ? 1 : 0;
 >     }
 >   });
 > }}}
 >
 > (As an aside, the comparison of `long` values should be accomplished
 with `Long.compare()` and analogously with other numbers.)
 >
 > The used comparison is not consistent with 'equals', as no other
 measures are taken (except partially in `addToHistory`) this could lead to
 an inconsistent history list. The test `compareTest` prints an example
 (cf. the key value combination).

 Yep, I can see us changing this towards using `Long.compare()` and also
 being more consistent with `equals()` and the like.

 > 3. So, the comparator (and related code) needs to be revised in order to
 reflect all the assumptions that can be inferred from `addToHistory` and
 `compressHistory`. Here the beginning of such a list:
 > * All history intervals at most overlap in one point or have an empty
 intersection.  If the overlap contains more than a point the interval is
 rejected (should such a rejection be logged?).

 This seems correct.  I can't say whether a rejection should be logged,
 because sometimes it might be useful to simply attempt to add an interval
 and not have the code yell if that is not possible.

 > * History intervals are supposed to be a minimum size, which?

 Huh, why?

 > * what else?

 Fine question.  Nothing comes to mind, but that might change when
 reviewing the test class more closely.

 > 4. History compression:
 > Here the condition for compressing, i.e. averaging respective values:
 >
 > {{{
 > 1: lastEndMillis == startMillis &&
 > 2:          ((lastEndMillis - 1L) / intervalLengthMillis) ==
 > 3:          ((endMillis - 1L) / intervalLengthMillis) &&
 > 4:          lastMonthString.equals(monthString) &&
 > 5:          lastMissingValues == missingValues
 > }}}
 >
 > Looking more closely at lines 2 and 3. This condition evaluates to true,
 if
 > `endMillis - 1 == k *intervalLengthMillis + r_em` and
 > `lastEndMillis - 1 == k * intervalLengthMillis + r_lem`
 > for a suitable natural Number `k`, and `r_em`, `r_lem` natural Numbers
 strictly less than `intervalLengthMillis`.
 > With line 1 and the given that interval starts should be less than their
 ends (cf. wrong date test) this implies `r_em` >= `r_lem`
 > and lines 2, 3 are equivalent to `endMillis - lastEndMillis <
 intervalLengthMillis`.

 Errrr, can you give an example for when this could go wrong? :)  Or was
 that a suggestion for rewriting that code to make it "even" more readable?

 > What did I overlook and what other assumptions are not reflected in the
 current code?

 Again, can't say off the top of my head.

 > 5. I didn't yet check. But, other comparator implementations and
 histories should be checked, too.  All these could be the cause of
 inconsistencies.  Add new trac issues for that?

 `WeightsStatus` "shares some code" with other classes in the same package.
 The reason is that it was easier to take an existing class and change it
 as needed than generalizing and writing subclasses.  I'd say let's clean
 up `WeightsStatus` first and then see what changes we can adapt in other
 classes.  In theory, by simplifying `WeightsStatus`, we'll get closer to a
 state where we can generalize classes and put common parts in a
 superclass.  But let's not aim for that just yet, if that's okay for you?

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