#22139: last_restarted and platform missing even though it is available in descriptor ---------------------------------+----------------------------------- Reporter: cypherpunks | Owner: metrics-team Type: defect | Status: new Priority: Medium | Milestone: metrics-lib 1.9.0 Component: Metrics/metrics-lib | Version: Severity: Normal | Resolution: Keywords: | Actual Points: Parent ID: | Points: Reviewer: | Sponsor: ---------------------------------+----------------------------------- Changes (by karsten):
* milestone: metrics-lib 1.8.0 => metrics-lib 1.9.0 Comment: I looked more at this issue and believe that it's not exactly a mad bug but a rather sad design decision. Let me explain. The situation is that we have a descriptor file starting with a truncated descriptor D,,0,, followed by several complete descriptors D,,1,, to D,,n,,. In this specific case D,,0,, is truncated in the middle of a base64-encoded digest string that we're trying to validate, so we're throwing a `DescriptorParseException`. But we're not catching that exception and moving on to D,,1,,. Instead, we're catching the exception in `DescriptorReader` where we're passing that exception to `DescriptorFileImpl#setException()` and not touching `DescriptorFileImpl#setDescriptors()` at all. Note that we would have done the exact same thing when running into a `DescriptorParseException` in D,,n,,! As a result, a `DescriptorFile` either contains parsed descriptors or an exception. All or nothing. This is not a bug, because we never claimed that we're returning parsed descriptors even if we run into an exception. That would have been the better design: just skip the descriptor we can't parse, set the exception if it's the first exception we ran into (as documented), and then move on to the next descriptor. But we can't say that the current implementation is incorrect. Even worse, if we change this behavior now, that might be considered a backward-incompatible change. History lesson: when metrics-lib was designed, descriptors were usually stored as one descriptor per file. We later switched to concatenating server descriptors and extra-info descriptors because handling many small files was less efficient. And even later, last year, we considered concatenating even more descriptors including votes. But that was all not the case when metrics-lib came to life. End of history lesson. My suggestion would be to implement #22141 first, so that the smallest returned unit is a `Descriptor`, not a `DescriptorFile`. We can fail a single descriptor that we can't parse, but we don't have to fail all other descriptors that happen to be contained in the same descriptor file. I'm changing the milestone to 1.9.0 for now. -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/22139#comment:4> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online _______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs