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

Added default muxer in case when none was provided. #264

Merged
merged 1 commit into from
Jan 28, 2018

Conversation

dmbreaker
Copy link

Should fix 'echo' example.

@dmbreaker
Copy link
Author

Ok, it will allow example pass, and won't display stacktrace.
But example will use another (default) protocol instead.

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

i think it would be better to just fix the example to specify a muxer?
Sounds like it's a required config argument.

// If secio is disabled, don't add our private key to the peerstore
if !cfg.DisableSecio {
ps.AddPrivKey(pid, cfg.PeerKey)
ps.AddPubKey(pid, cfg.PeerKey.GetPublic())
}

swrm, err := swarm.NewSwarmWithProtector(ctx, cfg.ListenAddrs, pid, ps, cfg.Protector, cfg.Muxer, cfg.Reporter)
swrm, err := swarm.NewSwarmWithProtector(ctx, cfg.ListenAddrs, pid, ps, cfg.Protector, muxer, cfg.Reporter)
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks inconsistent.

Copy link
Member

Choose a reason for hiding this comment

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

Not really. With the protector/reporter, we default to nil. This is the one case where we don't.

@@ -180,13 +180,19 @@ func newWithCfg(ctx context.Context, cfg *Config) (host.Host, error) {
ps = pstore.NewPeerstore()
}

// Set default muxer if none was passed in
muxer := cfg.Muxer
Copy link
Contributor

Choose a reason for hiding this comment

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

for consistency with the code below, it would be better to set it in the cfg object directly:

if cfg.Muxer == nil {
   cfg.Muxer = DefaultMuxer()
}

Copy link
Member

Choose a reason for hiding this comment

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

Hm. But that would modify the user's config. I prefer it this way.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -180,13 +180,19 @@ func newWithCfg(ctx context.Context, cfg *Config) (host.Host, error) {
ps = pstore.NewPeerstore()
}

// Set default muxer if none was passed in
muxer := cfg.Muxer
Copy link
Member

Choose a reason for hiding this comment

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

Hm. But that would modify the user's config. I prefer it this way.

// If secio is disabled, don't add our private key to the peerstore
if !cfg.DisableSecio {
ps.AddPrivKey(pid, cfg.PeerKey)
ps.AddPubKey(pid, cfg.PeerKey.GetPublic())
}

swrm, err := swarm.NewSwarmWithProtector(ctx, cfg.ListenAddrs, pid, ps, cfg.Protector, cfg.Muxer, cfg.Reporter)
swrm, err := swarm.NewSwarmWithProtector(ctx, cfg.ListenAddrs, pid, ps, cfg.Protector, muxer, cfg.Reporter)
Copy link
Member

Choose a reason for hiding this comment

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

Not really. With the protector/reporter, we default to nil. This is the one case where we don't.

@Stebalien
Copy link
Member

@vyzo given that we provide the DefaultMuxer method, I see no reason not to use it here.

@vyzo
Copy link
Contributor

vyzo commented Jan 24, 2018

fair enough.

@Stebalien Stebalien merged commit b624fe6 into libp2p:master Jan 28, 2018
@Stebalien
Copy link
Member

Thanks!

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