search

SlyMarbo / spdy

[deprecated] A full-featured SPDY library for the Go language.
BSD 2-Clause "Simplified" License
116 stars 13 forks source link

"common" package is awkward for callers #61

Open rnapier opened 10 years ago

rnapier commented 10 years ago

The new common package leads to the type common.Conn, common.Receiver, etc. exposed to the caller. That's a very awkward name. The caller expects these to be in the spdy package. I believe everything in the common package should be moved to the top-level spdy.

(I'm aware of the circular dependency. I'm working through some solutions to that.)

As I dig into this more, I realize the problem. Go packages don't exist for source code organization. They only exist for managing independent modules. There shouldn't be a spdy2 directory unless it is possible to build spdy without it. I actually would prefer that (I don't want to support spdy2 and don't want the code), but it's not how the code is currently structured.

The simplest solution is to move the subdirectories back into the root directory to match how the code currently works. I believe that should happen now; the current structure is just too confusing on callers IMO.

My recommendation for making it work better is this:

There should be a spdy.Protocol interface implemented by spdy2.Prococol and spdy3.Protocol. spdy.New*Conn() should take a slice of Protocols.

In each place that the protocol is checked (in RoundTrip() for instance), each Protocol should be asked if it can handle it, and if it can, it should be handed off.

A default list of all protocols could be returned from spdyutil so that callers don't have to construct their own list, but callers like me could construct their own list if they want to limit the protocols they support.

I can put together a PR for this recommendation over the next week or so.

SlyMarbo commented 10 years ago

I agree with the principle of keeping things together, but it makes the codebase much harder to navigate (and makes the naming quite messy). Bear in mind that you can still disable SPDY/2 support with spdy.DisableSpdyVersion(2).

Another option would be some kind of registration interface where you add support for different protocol versions by importing those packages, but then most users would need to import three packages (spdy, spdy/spdy2, and spdy/spdy3), which is far from ideal.

The main issue is the circular import you mention. If it weren't for that, I would've left what's currently in spdy/common/{interfaces.go, types.go, errors.go} in spdy. I'm not wildly happy with it either, and am working on some kind of solution.

However, most users shouldn't need to import spdy/common, since the core public API is in spdy still. Since common.Receiver is an interface, you should be able to define your own (or use nil) without importing the package.

I'll continue to work on a better structure, but I'm really not keen to move all of spdy2 and spdy3 back into spdy, since it'll result in an unpleasant amount of clutter.

rnapier commented 10 years ago

I went down this same road because I also am working on a large project. The answer after much gnashing of teeth was to put all the files that have to build together into a single directory. That's what the golang-nuts recommend as well. Only make directories for pieces you can build separately. The clutter is worth it to avoid the headaches and fragility (small changes reintroduce circular dependencies, which then requires API changes to fix). I went round and round; in the end, one directory has been much better.

Regarding impact on callers, I need access to the spdy.Conn for dealing with the proxy code. That's where I wound up with common.Conn (which is confusing very confusing). You have the same problem in examples/proxy_server/proxy_server.go.

SlyMarbo commented 10 years ago

Yeah, I see what you mean. I'm currently in the process of decoupling spdy2 and spdy3 from common in the hope that they can be used independently. That way, you can use spdy if you don't care much about the internals, or you can use a specific package on its own. In your case, you should be able to use just spdy3.

One part of this is to separate out functionality. Rather than Conn being a massive interface, it should just have the bare essentials, with extra interfaces for other stuff. This should make it easier to decouple the different packages.

On 10 June 2014 19:25, Rob Napier notifications@github.com wrote:

I went down this same road because I also am working on a large project. The answer after much gnashing of teeth was to put all the files that have to build together into a single directory. That's what the golang-nuts recommend as well. Only make directories for pieces you can build separately. The clutter is worth it to avoid the headaches and fragility (small changes reintroduce circular dependencies, which then requires API changes to fix). I went round and round; in the end, one directory has been much better.

Regarding impact on callers, I need access to the spdy.Conn for dealing with the proxy code. That's where I wound up with common.Conn (which is confusing very confusing). You have the same problem in examples/proxy_server/proxy_server.go.

— Reply to this email directly or view it on GitHub https://github.com/SlyMarbo/spdy/issues/61#issuecomment-45652192.