Opened 9 years ago

Closed 8 years ago

#2557 closed defect (fixed)

BridgeDB fails with a traceback when turning off a distributor

Reported by: karsten Owned by: nickm
Priority: Medium Milestone:
Component: Circumvention/BridgeDB Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

BridgeDB breaks with a traceback when one first runs it with a distributor turned on, then reconfigures it to turn that distributor off, and then runs it again. It doesn't know where to put the bridges from the database.

Child Tickets

Change History (8)

comment:1 Changed 8 years ago by kaner

Status: newneeds_review

Proposed fix in c518422f708e5d059f3d77e882e1da04b38d30c1 in git://git.torproject.org/kaner/bridgedb.git
Disabled distributors are re-assigned to "unallocated" now.
Tested on my local system. Nick, please review and merge if it is ok.

comment:2 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Sure, let's give it a shot. Merging.

comment:3 Changed 8 years ago by karsten

Resolution: fixed
Status: closedreopened

Patch looks good. Two minor comments:

  • I'm not 100% certain about the new semantics of insertBridgeAndGetRing(). What's a "validated ring"? And should the comment mention that we're returning defaultPool if the new ring (or the ring in the database?) is not contained in validRings?
  • I wonder why the changes in Tests.py work at all. If there are only ring1, ring2, and ring3 and we insert a bridge into ring10, how does it end up in ring1 and not in unallocated?

comment:4 Changed 8 years ago by kaner

Thanks for reviewing. Please see commit 48527eaf4686a2c4f436b3e0e17d6c7fb7c79ba5 for a minor fix. (Merge, close if ok)

comment:5 Changed 8 years ago by nickm

karsten, if you like the revision, feel free to merge it.

comment:6 in reply to:  4 ; Changed 8 years ago by karsten

Replying to kaner:

Thanks for reviewing. Please see commit 48527eaf4686a2c4f436b3e0e17d6c7fb7c79ba5 for a minor fix. (Merge, close if ok)

The tests now fail for me:

FAIL: testBridgeStorage (bridgedb.Tests.SQLStorageTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "build/lib/bridgedb/Tests.py", line 164, in testBridgeStorage
    self.assertEquals(r, "unallocated")
AssertionError: u'ring1' != 'unallocated'

And that's what I don't understand. Your change to the tests in 48527ea looks correct to me. So, maybe there's something wrong in the implementation? Also note that there's another test right after adding a bridge to ring99 that needs to be changed.

comment:7 in reply to:  6 Changed 8 years ago by kaner

Replying to karsten:

And that's what I don't understand. Your change to the tests in 48527ea looks correct to me. So, maybe there's something wrong in the implementation? Also note that there's another test right after adding a bridge to ring99 that needs to be changed.

Ok. Sorry. I should have checked your initial comment about "how does it end up in ring1 and not in unallocated" better. Of course the bridge ends up in ring1, because it initially gets assigned to ring1 by the first call to insertBridgeAndGetRing. Its the *same* bridge that goes in. Bridges aren't moved between rings. Only the time stamp gets updated the second time it goes in. So it was correct from the beginning on. Same goes for the bridge that wants to go into ring99 btw. Same key, same ring. All ring1.

Feel free to revert part of the last commit or simply use 971f8aee61a03eefde45ffd319eff646e6e8252e.

comment:8 Changed 8 years ago by karsten

Resolution: fixed
Status: reopenedclosed

Okay, sorry for the confusion. I squashed your two commits and pushed them to master. Closing. Thanks!

Note: See TracTickets for help on using tickets.