nskinkel / oppy

A Tor client (onion proxy) implementation in Python
BSD 3-Clause "New" or "Revised" License
24 stars 3 forks source link

Misc to merge #32

Closed dwtj closed 10 years ago

dwtj commented 10 years ago

NetStatus and ServerDescriptors

I think that Nik merged the recent work of Patrick and Zach into this branch recently. You guys know the specifics of this better than I do.

Imports

As development has happened, various work has been done to simplify imports to prevent circular imports. One key change is that some small classes with few dependencies that are used widely across the project (most prominently, forward.Forwarder) have been moved to oppy.util. By putting them here, they can be imported from across the project with few side effects.

Globally shared instances have been moved to oppy.shared. This prevents the globally shared objects from being instantiated as a side effect of importing other packages. Since any NetStatus instance sets indefinite updates, this is crucial for making unit tests work.

As Nik has pointed out, there is still more to do on this matter, however. The most important change is the near future is to refactor the entire project to restrict the use of __init__.py. The potential for circular references and their complexity is not worth it.

Unit Tests

All unit tests run. However, small portions of unit tests which expect NetStatus to be able to return an OnionPath of RelayDescriptor objects have been commented out to make the test suite run. This functionality is coming soon along with Nik's other work refactoring NetStatus

I changed unit tests which require a NetStatus so that they create a new one for every test. My experience, experiments, and readings indicate that doing so is important for maintaining consistent testing behavior. (The one exception is the unit test meant to test oppy.shared.net_status.) Note that you can clean a dirty reactor after a test using oppy.tests.util.clean.cancelAllPendingCalls().

CircuitHandshake

Work on this has been begun. The structure and interfaces are in place. In particular, see the contents of oppy.circuit.handshake, crypto.cryptostore, and crypto.cryptomaterial.

The old crypto work still needs to be adapted to work from within these components.

dwtj commented 10 years ago

For some initial work on the process of removing imports from various __init__.py files, see the branch RmInitPyImports. Currently, all imports have been removed from cell/__init__.py, and all cell-related imports have been updated to the new style.

nskinkel commented 10 years ago

Here's a few big issues I can see right off the bat. Haven't had a chance to look at actual code much, but I will over the next day or two. Here's a summary of the immediate problems, in rough order of precedence.

Server Descriptor Includes

There is a bunch of code included from the ServerDescriptor branch. Some of us are still working on this, and it's definitely not ready to be merged into master yet.

Missing Sphinx Docs

There are a bunch of places that are either missing docs or have docs formatted in a non-sphinx way.

Import/init problems

We should really just go ahead and fix this stuff now. Putting it off is only going to make it worse.

Lack of issues

There aren't (as far as I can tell) open issues associated with lots of the code being merged here -- we need to make sure we're documenting simplifications and violations of the protocol we make. Again, this will become totally unmanageable if we put it off.

That's just a summary of high-level stuff. I'll check out the code and post again. But I'm definitely not comfortable with this code going into master until these issues are fixed.

dwtj commented 10 years ago
  1. Server Descriptor Includes: That is completely your prerogative. We'll hold off until the NetStatus-related stuff is in the shape that you want it.
  2. Missing Sphinx Docs: I agree completely that we should fill these in properly.
  3. Import/Init Problems: I am working on this now. The work is going into the RmInitPyImports branch.
  4. Lack of Issues:: I think that this also needs to be done properly.

Though I agree that (2) and (4) should be done before merging anything else into master, I do not think that we should not focus on these tasks until we get through our push for the demo.

Therefore, I propose the following: how about I close this pull request for now, and we rename this branch Demo. Then, Nik's work on the NetStatus and Zach/Patrick's current work on cells can be merged into this branch.

From that point, we can make branches off of Demo to add features such as CircuitCrypto, ConnectionFSM, and ConnectionCrypto.

This way we can make a stab at getting something done for the demo, and put off Nik's items(2) and item (4). After the demo we can re-consider where we are at, tidy things up, and merge things into master once they are solid.

Once Demo is in master, then we can focus on additional features through additional branches.

dwtj commented 10 years ago

The work of removing imports from all __init__.py files is complete. It was done on RmInitPyImports. That branch was merged with this branch, MiscToMerge, and then deleted.

dwtj commented 10 years ago

After talking with Nik this evening, we came up with a concrete plan for the demo. He will share this later. As I proposed earlier, this pull request is being closed, and MiscToMerge is being renamed Demo. In preparation for the demo,w e can make branches off of Demo to add features such as CircuitCrypto, ConnectionFSM, and ConnectionCrypto.