[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #9264 [BridgeDB]: Problem with transport lines in BridgeDB's bridge pool assignment files
#9264: Problem with transport lines in BridgeDB's bridge pool assignment files
--------------------------+--------------------------------------------
Reporter: karsten | Owner: isis
Type: defect | Status: needs_review
Priority: major | Milestone:
Component: BridgeDB | Version:
Resolution: | Keywords: bridgedb-0.1.3, bridgedb-0.1.5
Actual Points: | Parent ID:
Points: |
--------------------------+--------------------------------------------
Changes (by sysrqb):
* status: needs_revision => needs_review
Comment:
Replying to [comment:34 isis]:
> Comments:
>
> 1. In
[https://gitweb.torproject.org/user/sysrqb/bridgedb.git/commitdiff/ff3a2641ac3447238511ce3436386d9ee2553011
commit ff3a2641ac3447238511ce3436386d9ee2553011]... somewhere a while ago
(not in this commit specifically) I think the `COLLECT_TIMESTAMPS` option
for #10724 got lost. I think I remember seeing it re-added in your #5232
branch though.
>
Darn. It looks like it was dropped in
967f1a6a4290154977ab7c0cc2cbe0560d7246ce :( not good. I'll fix this
(unless you beat me to it).
> 2. In 13fc557b8e4bd094457ca954129dceaa4445d744, the
`test_splitterBridgeInsertion` is great!
>
> 3. In 92eb6fe9e7d4e1e7cbb72687b9f41f5f827fa182:
>
> {{{
> - self.bridges.append(bridge)
> + index = 0
> + logging.debug("Inserting %s into %s ring" %
(bridge.fingerprint, self.key))
> + for old_bridge in self.bridges:
> + if bridge.fingerprint == old_bridge.fingerprint:
> + self.bridges[index] = bridge
> + break
> + index += 1
> + else:
> + self.bridges.append(bridge)
> }}}
>
> Don't you want to use `Util.logSafely()`?
Probably a good idea for the fingerprint. I removed the `key` because it
was unhelpful.
> And doesn't the `self.bridges[index] = bridge` mean that, if there were
already duplicates in `self.bridges`, that only the first duplicate is
replaced and the others remain in the list?
>
yes, however, 1) this should also prevent duplicates from being inserted
in the first place, and 2) because we only insert bridges if they are
defined in the NS we should never even try to insert the same bridge more
than once (for each call to Main.load()). If we were still unconditionally
parsing all descriptors then this would definitely be a concern. We only
see this bug on SIGHUP when we create a new, distinct, instance of Bridge
to represent a bridge in the splitter. If an instance of Bridge already
exists in the splitter for that bridge, then, previously, the new instance
was appended to the list (resulting in the splitter holding two instances
for the same bridge), now the new instance should overwrite the old
instance. They're distinct instances, so new != old, which is why we
compare fingerprints.
(sorry if you already understood this and I didn't understand the
question)
> Also, it's a bit odd, but you might want to do `for old_bridge in
self.bridges[:]:` instead of `for old_bridge in self.bridges:`, because of
a list comprehension bug in Python (I don't recall off the top of my head
what version it was fixed in...) where there is an off-by-one error in
calculating the number of elements in the list (but only with really large
lists like this one).
>
(4000 is really large? :)) Better to play it safe. Changed.
> 4. In general, don't worry about the space taken up by splitting
unittests up into many smaller tests, because the first thing that raises
an exception means all the rest of the test doesn't get run.
>
True, true
> ---------
>
> Other than the tiny things mentioned above, I think this should be
merged immediately for version 0.1.5.
Added two commits in
[https://gitweb.torproject.org/user/sysrqb/bridgedb.git/shortlog/refs/heads/bug9264_rebased_3_r1
bug9264_rebased_3_r1], which I think solve the above comments.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9264#comment:35>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs