Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor net code to use transports, in rough accordance with libp2p #1937

Merged
merged 4 commits into from
Nov 10, 2015

Conversation

whyrusleeping
Copy link
Member

refactor as discussed with @jbenet. (with a couple extra methods on the interfaces)

This has the nice effect of removing go-multiaddr-net imports from a lot of places.

License: MIT
Signed-off-by: Jeromy jeromyj@gmail.com

@jbenet jbenet added the status/in-progress In progress label Nov 3, 2015
@whyrusleeping
Copy link
Member Author

@jbenet one question I still have is how to wire timeouts into the Dialer through this interface.

@jbenet
Copy link
Member

jbenet commented Nov 3, 2015

@whyrusleeping probably extend it to have the same vars? not sure.

@whyrusleeping
Copy link
Member Author

@jbenet but its an interface... Its going to have to be one of:

type Transport interface {
    Dialer(la ma.Multiaddr, opts ...[]DialOpts) (Dialer, error)
....
}

or

type Dialer interface {
    DialTimeout(a ma.Multiaddr, t time.Duration) (Conn, error)
...
}

@cryptix
Copy link
Contributor

cryptix commented Nov 3, 2015

type Transport interface {
    Dialer(la ma.Multiaddr, opts ...DialOpts) (Dialer, error)
}

type DialOpts func(*Transport) error

func SetTimeout(t time.Duration) DialOpts {
    return func(tr *Transport) error {
    // setup timeout on tr
    }
}

seems more flexible to me.

@whyrusleeping
Copy link
Member Author

@cryptix i'd like to be able to use the functional options, but in this case its non-trivial (and more trouble than its worth).

Each implementation of a transport dialer may want different options, and have different ways of setting them, so we cant nicely make it a method on the interface.

@cryptix
Copy link
Contributor

cryptix commented Nov 3, 2015

Couldn't you type assert to other interface in SetTimeout like:

func SetTimeout(t time.Duration) DialOpts {
    return func(tr *Transport) error {

        switch v := tr.(type) {
        case ma.Timeouter:
            _ = v.SetTimeout(t)

        case net.Dialer:
            d := net.Dialer{Timeout: timeout}
            tr.UseDialer(d)
        }
    }
}

@daviddias
Copy link
Member

@whyrusleeping #1937 (comment)

Can't it be one of the opts when you instantiate the Transport for the first time?

@whyrusleeping
Copy link
Member Author

@diasdavid yeah... it could be. but then that makes it pretty difficult to change the settings later on if needed.

return newMeteredConn(c, bwc.LogRecvMessage, bwc.LogSentMessage)
}

func newMeteredConn(base manet.Conn, rcb metrics.MeterCallback, scb metrics.MeterCallback) manet.Conn {
func newMeteredConn(base transport.Conn, rcb metrics.MeterCallback, scb metrics.MeterCallback) transport.Conn {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is MeteredConn a 'Connection Upgrade' that dumps bandwidth metrics? This is really a great idea!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeap!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want it the js stuff! :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diasdavid just another transport :)

@whyrusleeping
Copy link
Member Author

Once we merge this, rebase 0.4.0 and extract libp2p, i'm going to grab most of the actual 'networking' code from multiaddr-net and put it in the transport lib, leaving all of the multiaddr parsing/handling code there.


type Transport interface {
Dialer(laddr ma.Multiaddr, opts ...DialOpt) (Dialer, error)
Listener(laddr ma.Multiaddr) (Listener, error)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just Listen to match go? not sure. we can always change it later.

@jbenet
Copy link
Member

jbenet commented Nov 8, 2015

@whyrusleeping this LGTM! great, great changeset. Really happy with how this interface turned out.

only major thing to fix is:

  • restore the old retErr behavior that does not stop the listen/setup method on Listen failure.

minor:

  • maybe Transport.Listen instead of Transport.Listener ?
  • maybe Swarm.setupInterfaces or just Swarm.setup ?

@whyrusleeping
Copy link
Member Author

alright, we no longer fail out on swarm setup unless all listens fail.

@daviddias daviddias mentioned this pull request Nov 9, 2015
11 tasks
@whyrusleeping whyrusleeping force-pushed the refactor/transport branch 2 times, most recently from 22c41ea to efd0753 Compare November 10, 2015 03:44
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
And other assorted PR feedback

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
jbenet added a commit that referenced this pull request Nov 10, 2015
refactor net code to use transports, in rough accordance with libp2p
@jbenet jbenet merged commit ece43a5 into master Nov 10, 2015
@jbenet jbenet deleted the refactor/transport branch November 10, 2015 04:41
@daviddias
Copy link
Member

It got merged, wooo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants