Remove muxer-multistream.Transport and refactor upgrader #1861

julian88110 · 2022-11-08T22:30:07Z

Details can be found in Issue #1854.

Remove muxer-multistream.Transport as an entity of type network.Multiplexer.
Define a new type MsTransport to encapsulate the multistream selection function.
Revise upgrader code to use the new MsTransport for muxer negotiation.
Update the relevant tests.

- Decoulple multistream.Transport from network.Multiplexer. - Introduce MsTransport to encapsulate muxer negotiation function - Refactor code to make it more streamlined. - Update tests to use the new MsTransport.

marten-seemann

I don't think we should have the MsTransport interface, or its implementation. The point of #1854 was to get rid of this complexity, not to move it to a different package.

p2p/net/upgrader/upgrader_test.go

p2p/net/upgrader/multistream.go

Co-authored-by: Marten Seemann <[email protected]>

p2p/net/upgrader/multistream.go

MarcoPolo

I agree with @marten-seemann , let's remove the muxer_multistream package. I left some notes that I think give a rough idea of the steps required. But really you can probably start by copying the NegotiateMuxer func, deleting the muxer_multistream package, and then fixing errors/tests.

You got yourself a great refactor here! and I would expect net negative code and fewer public things :)

p2p/transport/websocket/websocket_test.go

MarcoPolo · 2022-11-10T01:05:09Z

p2p/net/upgrader/upgrader_test.go


-func (m *errorMuxer) NewConn(c net.Conn, isServer bool, scope network.PeerScope) (network.MuxedConn, error) {
+func (m *errorMuxer) NegotiateMuxer(c net.Conn, isServer bool) (*upgrader.Multiplexer, error) {


This is a different test now. The old test errored when creating a muxed connection. This now errors when negotiation. I don't think we should change this test, but it would maybe we should add another test if it makes sense.

As the multistream transport itself is removed, this test is no longer relevant so I think it should be removed now.

MarcoPolo · 2022-11-10T01:09:14Z

p2p/net/upgrader/upgrader.go

 	go func() {
 		defer close(done)
-		smconn, err = u.muxer.NewConn(conn, server, scope)
+		streamMuxer, err = u.mstream.NegotiateMuxer(conn, server)


I think NegotiateMuxer should take a ctx and then we don't have to do this select dance below and we don't have to spawn a go routine.

That's not possible since the MultistreamMuxer doesn't take a context (see my comment in https://github.com/libp2p/go-libp2p/pull/1861/files#r1018835597). Let's not touch timeouts in this PR.

MarcoPolo · 2022-11-10T01:15:40Z

p2p/net/upgrader/multistream.go

 	t.mux.AddHandler(path, nil)
 	t.tpts[path] = tpt
 	t.OrderPreference = append(t.OrderPreference, path)
 }

-func (t *Transport) NewConn(nc net.Conn, isServer bool, scope network.PeerScope) (network.MuxedConn, error) {
+func (t *Transport) NegotiateMuxer(nc net.Conn, isServer bool) (*Multiplexer, error) {


This is the only function you need – and it's just a thin wrapper around go-multistream. I would say:

we move this to the upgrader (No one else really cares about this package (I think)).

Use a ctx to set the negotiate timeout

Make it private

Delete this whole package. less code!

Agreed, see https://github.com/libp2p/go-libp2p/pull/1861/files#r1017265425.

I wish that was possible, but unfortunately the MultistreamMuxer doesn't take a context (because there's no io.Reader that takes a context), so we can't use a context here. Anyway, fixing this seems orthogonal to this PR.

Yes please!

I assume you mean struct, not package. This code is now living in the upgrader package :)

marten-seemann

This looks a lot better than the last iteration!

marten-seemann · 2022-11-15T02:06:07Z

p2p/net/upgrader/upgrader.go

@@ -74,15 +75,21 @@ type upgrader struct {
 	//
 	// If unset, the default value (15s) is used.
 	acceptTimeout time.Duration
+
+	msmuxer    *mss.MultistreamMuxer //


Empty comment?

marten-seemann · 2022-11-15T02:07:03Z

p2p/net/upgrader/upgrader.go

 }

 var _ transport.Upgrader = &upgrader{}

-func New(secureMuxer sec.SecureMuxer, muxer network.Multiplexer, opts ...Option) (transport.Upgrader, error) {
+func New(secureMuxer sec.SecureMuxer, muxers map[string]network.Multiplexer, preference []string, opts ...Option) (transport.Upgrader, error) {


These constructor options are very unclean.

What you want to use here is list of <id>, <muxer>, ordered by preference. Introduce a type that reflects that.

marten-seemann · 2022-11-15T02:10:15Z

p2p/net/upgrader/upgrader.go

@@ -74,15 +75,21 @@ type upgrader struct {
 	//
 	// If unset, the default value (15s) is used.
 	acceptTimeout time.Duration
+
+	msmuxer    *mss.MultistreamMuxer //
+	muxers     map[string]network.Multiplexer


Given that in most cases we'll only have 2 muxers, it's probably faster to just iterate over a slice. Map lookups only amortize for larger sets.

You can make the (internal) API look a bit nicer by introducing a getMuxerByID() (*multiplexer, bool) method.

marten-seemann · 2022-11-15T02:12:10Z

p2p/net/upgrader/upgrader.go

-		smconn, err = u.muxer.NewConn(conn, server, scope)
+		muxer, err = u.negotiateMuxer(conn, server)
+
+		if err == nil {


Nit: Return early if err != nil.

marten-seemann · 2022-11-15T02:12:40Z

p2p/net/upgrader/upgrader.go

@@ -26,6 +26,8 @@ var ErrNilPeer = errors.New("nil peer")
 // AcceptQueueLength is the number of connections to fully setup before not accepting any new connections
 var AcceptQueueLength = 16

+var defaultNegotiateTimeout = time.Second * 60


Suggested change

var defaultNegotiateTimeout = time.Second * 60

const defaultNegotiateTimeout = time.Second * 60

marten-seemann · 2022-11-15T02:17:28Z

p2p/net/upgrader/upgrader_test.go

 	priv, _, err := test.RandTestKeyPair(crypto.Ed25519, 256)
 	require.NoError(t, err)
 	id, err := peer.IDFromPrivateKey(priv)
 	require.NoError(t, err)
-	u, err := upgrader.New(&MuxAdapter{tpt: insecure.NewWithIdentity(id, priv)}, muxer, opts...)
+	muxerId := "/yamux/1.0.0"


Please stick to Go naming conventions.

Suggested change

muxerId := "/yamux/1.0.0"

muxerID := "/yamux/1.0.0"

marten-seemann · 2022-11-15T02:17:58Z

p2p/net/upgrader/upgrader_test.go

@@ -163,25 +114,6 @@ func TestOutboundResourceManagement(t *testing.T) {
 		require.NoError(t, conn.Close())
 	})

-	t.Run("failed negotiation", func(t *testing.T) {


Why are you deleting this test case?

marten-seemann · 2022-11-15T02:19:24Z

config/muxer.go

 	transportSet := make(map[string]struct{}, len(tpts))
+	muxers := make(map[string]network.Multiplexer)
+	preference := make([]string, 0)


Preallocate slices with the correct capacity, if you know the capacity in advance.

julian88110 added 2 commits November 7, 2022 13:53

Remove mulstistream.Transport entity refactor multistream code

597a711

- Decoulple multistream.Transport from network.Multiplexer. - Introduce MsTransport to encapsulate muxer negotiation function - Refactor code to make it more streamlined. - Update tests to use the new MsTransport.

Clean up code and run full test

0e63f4f

julian88110 requested a review from marten-seemann November 8, 2022 22:30

marten-seemann requested changes Nov 9, 2022

View reviewed changes

p2p/net/upgrader/upgrader_test.go Outdated Show resolved Hide resolved

p2p/net/upgrader/multistream.go Outdated Show resolved Hide resolved

p2p/net/upgrader/multistream.go Outdated Show resolved Hide resolved

julian88110 and others added 2 commits November 9, 2022 09:24

Update p2p/net/upgrader/multistream.go

b8c5b17

Co-authored-by: Marten Seemann <[email protected]>

address review points

3a2cfea

julian88110 self-assigned this Nov 9, 2022

julian88110 requested a review from MarcoPolo November 9, 2022 23:33

MarcoPolo reviewed Nov 10, 2022

View reviewed changes

p2p/net/upgrader/multistream.go Outdated Show resolved Hide resolved

MarcoPolo requested changes Nov 10, 2022

View reviewed changes

p-shahi linked an issue Nov 12, 2022 that may be closed by this pull request

Remove multistream.transport as an entity of type network.multiplexer #1854

Closed

julian88110 added 2 commits November 13, 2022 11:05

Remove MsTransport and merge its function into upgrader.

e4a1c2a

fix some tests

48cfeb3

marten-seemann reviewed Nov 15, 2022

View reviewed changes

marten-seemann mentioned this pull request Nov 16, 2022

upgrader: absorb the muxer_multistream.Transport into the upgrader #1885

Merged

marten-seemann closed this in #1885 Nov 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove muxer-multistream.Transport and refactor upgrader #1861

Remove muxer-multistream.Transport and refactor upgrader #1861

julian88110 commented Nov 8, 2022

marten-seemann left a comment

MarcoPolo left a comment

MarcoPolo Nov 10, 2022

julian88110 Nov 14, 2022

MarcoPolo Nov 10, 2022

marten-seemann Nov 10, 2022

MarcoPolo Nov 10, 2022

marten-seemann Nov 10, 2022

marten-seemann left a comment

marten-seemann Nov 15, 2022

marten-seemann Nov 15, 2022

marten-seemann Nov 15, 2022

marten-seemann Nov 15, 2022

marten-seemann Nov 15, 2022

marten-seemann Nov 15, 2022

marten-seemann Nov 15, 2022

marten-seemann Nov 15, 2022


		func (m *errorMuxer) NewConn(c net.Conn, isServer bool, scope network.PeerScope) (network.MuxedConn, error) {
		func (m errorMuxer) NegotiateMuxer(c net.Conn, isServer bool) (upgrader.Multiplexer, error) {

	var defaultNegotiateTimeout = time.Second * 60
	const defaultNegotiateTimeout = time.Second * 60

Remove muxer-multistream.Transport and refactor upgrader #1861

Remove muxer-multistream.Transport and refactor upgrader #1861

Conversation

julian88110 commented Nov 8, 2022

marten-seemann left a comment

Choose a reason for hiding this comment

MarcoPolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marten-seemann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment