Extract DHT message sender from the DHT #659

aschmahmann · 2020-06-01T19:03:42Z

This can be used to make life easier for crawlers

aschmahmann · 2020-06-01T20:06:31Z

dht_net.go

@@ -208,12 +210,38 @@ func (dht *IpfsDHT) handleNewMessage(s network.Stream) bool {
 	}
 }

+type messageManager struct {


better name welcome 😄

records.go

routing.go

go.sum

aarshkshah1992

Definitely makes the DHT API easier to understand.

dht_net.go

aarshkshah1992 · 2020-06-03T07:30:33Z

dht_net.go

@@ -208,12 +210,38 @@ func (dht *IpfsDHT) handleNewMessage(s network.Stream) bool {
 	}
 }

+type messageManager struct {


@aschmahmann I am not entirely sure we need this indirection. Why not just stick all this functionality on the ProtocolMessenger ?

Mostly because I'm not really a fan of adding in all the complexity of the stream management into the a simple wrapper for sending protocol messages, but it was the easiest way to do it for now.

It'd be nice to make that all optional in the future, but the stream management and stream parsing are bit mixed together at the moment in the form of the messageManager struct.

To make this more clear I've now decoupled the messageManager from the ProtocolMessenger via an interface. This should give consumers of ProtocolMessenger more flexibility to do things like immediately close connections after sending messages if they're not interested in long-term connections.

aarshkshah1992 · 2020-06-03T07:33:27Z

subscriber_notifee.go

-		defer ms.lk.Unlock()
-		ms.invalidate()
-	}()
+	dht.protoMessenger.m.streamDisconnect(dht.Context(), p)


This is one of the reasons I don't like the messageManager indirection. Let's just keep all that on the ProtocolMessenger.

As mentioned above, the idea that this stream management optimization is tied into the protocol messenger is not optimal, it's just something to live with for now. Once we can dissociate the stream management from stream processing then it will not have as much indirection.

routing.go

aarshkshah1992 · 2020-06-03T07:53:13Z

routing.go

 	wg := sync.WaitGroup{}
 	for p := range peers {
 		wg.Add(1)
 		go func(p peer.ID) {
 			defer wg.Done()
 			logger.Debugf("putProvider(%s, %s)", keyMH, p)
-			err := dht.sendMessage(ctx, p, mes)
+			err := dht.protoMessenger.PutProvider(ctx, p, keyMH, dht.host)


@aschmahmann Instead of passing in the host and peerId to this function, why not change it to PutProviderRecord(ctx, peer.AddrInfo) and pass them from here ?

I'd like to avoid passing in the host if we can.

We can if you think so. However, I put in the host instead of just peer.AddrInfo to account for:

You cannot currently create a valid provider record for any peer other than host

When we have signed provider records we'll be able to get them from the host, but not from peer.AddrInfo

However, given that this is a "use me if you know what you're doing" function 1 isn't a huge concern. Additionally, we likely will need to add/change the function signature anyway once we have signed provider records to accommodate rebroadcasting other people's records.

LMK what you think

messages.go

wire/messages.go

aschmahmann

Did some more refactoring of this PR and broke things into commits in case we want to revert any one of them and to make it easier to review. Recommend reviewing by commit.

dht_net.go

wire/messages.go

willscott

🎉

…ct. create a new struct that manages sending DHT messages that can be used independently from the DHT.

…ng them into a new package

…valid record

willscott · 2021-01-04T17:13:22Z

dht.go

 		return nil, err
 	}

 	// Double check the key. Can't hurt.
 	if rec != nil && string(rec.GetKey()) != key {
-		logger.Errorw("BUG: found a DHT record that didn't match it's key", "expected", loggableRecordKeyString(key), "got", rec.GetKey())
+		logger.Errorw("BUG: found a DHT record that didn't match it's key", "expected", internal.LoggableRecordKeyString(key), "got", rec.GetKey())
 		return nil, nil


this.. seems like it's an error not a nil?

probably... I mean realistically it wouldn't even be crazy to panic here since if this occurs then there was a programming error somewhere.

While this shouldn't happen, returning nil, nil when there is a validation error does fulfill the function contract just like it does in dht.getRecordFromDatastore. I've copied the comment explaining that from dht.getRecordFromDatastore to here.

willscott · 2021-01-04T17:14:47Z

internal/errors.go

+
+import "errors"
+
+var ErrInvalidRecord = errors.New("received invalid record")


is this error never going to be returned to an external consumer? re-export it or have it defined somewhere that can be referenced externally preferably

It isn't currently. We were using and checking against it internally and I thought it'd be more recognizable when reading code to have it as a member of the internal package then as an unexported variable somewhere

message_manager.go

willscott · 2021-01-04T17:19:24Z

message_manager.go

+	}()
+}
+
+// SendRequest sends out a request, but also makes sure to


this is a somewhat weird place to be doing this. It

isn't DHT specific - why aren't we doing this at the host connection level?

feels potentially ugly with a general connection manager rather than factored out into a peer prioritization logic place.

I think the Ask is to think about what the ideal place for speed measurement logic to live, and file an issue for that (probably not in this repo.) I'm okay having it here for now

This was just a move from dht_net.go where it was previously. I tend to agree that I'm not sure why we're recording latency with m.host.Peerstore().RecordLatency(p, time.Since(start)) when it includes processing time on the server too.

The other metrics emitted here are DHT specific though.

willscott · 2021-01-04T17:22:14Z

message_manager.go

+	// We only want to speak to peers using our primary protocols. We do not want to query any peer that only speaks
+	// one of the secondary "server" protocols that we happen to support (e.g. older nodes that we can respond to for
+	// backwards compatibility reasons).
+	nstr, err := ms.m.host.NewStream(ctx, ms.p, ms.m.protocols...)


this is the only place that the message sender uses it's reference to the manager, and also the only place the host is really made use of.

Can we provide messageSender's with a closure for making new streams, rather than a full reference to the manager? making it clear that responsibility is manger->sender(s) will be clearer code than having back references.

We can, but we also need the host for the latency measurements above. I'm not sure who is using those metrics or why (perhaps @Stebalien has some ideas), so I left things the same (previously there was a reference to the IpfsDHT here).

pb/protocol_messenger.go

willscott · 2021-01-04T17:26:35Z

routing.go

@@ -329,7 +329,7 @@ func (dht *IpfsDHT) getValues(ctx context.Context, key string, stopQuery chan st
 					return nil, err
 				default:
 					return nil, err
-				case nil, errInvalidRecord:
+				case nil, internal.ErrInvalidRecord:


returning this where the consumer can't match on it seems bad

This is returned from an internal function and then swallowed, it shouldn't escape to the user.

…s to clarify what peerMessageSender and messageManager do

dht.msgSender

aschmahmann force-pushed the refactor/extract-messages branch from 24302d9 to 755d6df Compare June 1, 2020 19:04

aarshkshah1992 self-requested a review June 1, 2020 19:20

aschmahmann commented Jun 1, 2020

View reviewed changes

aschmahmann force-pushed the refactor/extract-messages branch from 755d6df to b918f42 Compare June 1, 2020 20:14

aschmahmann commented Jun 1, 2020

View reviewed changes

go.sum Outdated Show resolved Hide resolved

aarshkshah1992 suggested changes Jun 3, 2020

View reviewed changes

aschmahmann force-pushed the refactor/extract-messages branch 2 times, most recently from 1cbdbdf to 773aa01 Compare October 7, 2020 03:14

aschmahmann commented Oct 7, 2020

View reviewed changes

wire/messages.go Outdated Show resolved Hide resolved

aschmahmann force-pushed the refactor/extract-messages branch from b0dc23d to 197ecae Compare October 7, 2020 03:39

aschmahmann requested review from aarshkshah1992 and willscott October 7, 2020 04:01

aschmahmann commented Oct 7, 2020

View reviewed changes

wire/messages.go Outdated Show resolved Hide resolved

aschmahmann commented Oct 7, 2020

View reviewed changes

willscott reviewed Oct 7, 2020

View reviewed changes

dht_net.go Outdated Show resolved Hide resolved

dht_net.go Outdated Show resolved Hide resolved

wire/messages.go Outdated Show resolved Hide resolved

wire/messages.go Outdated Show resolved Hide resolved

wire/messages.go Outdated Show resolved Hide resolved

aschmahmann commented Oct 7, 2020

View reviewed changes

wire/messages.go Outdated Show resolved Hide resolved

willscott approved these changes Oct 12, 2020

View reviewed changes

aschmahmann force-pushed the refactor/extract-messages branch from eac5d7d to fcf7104 Compare October 12, 2020 18:33

aschmahmann added 11 commits January 4, 2021 00:52

refactor: extract messaging components from IpfsDHT into its own stru…

c9c088f

…ct. create a new struct that manages sending DHT messages that can be used independently from the DHT.

docs: cleaned up comments

a3c7c3d

refactor: decouple ProtocolMessenger from MessageSender implementation

94d08a2

refactor: temporarily export loggable bytes array aliases before movi…

7b5446a

…ng them into a new package

refactor: move logging helpers to internal package

7a80399

refactor: move invalid record error into internal package

52b4eb2

refactor: move ProtocolMessenger into new package

f73b905

refactor: return nil instead of an empty record when processing an in…

786c3c9

…valid record

refactor: move message manager to its own file

4b3a573

refactor: cleanup protocol messenger comments

9977256

refactor: move protocol messenger to pb package

138cb80

aschmahmann force-pushed the refactor/extract-messages branch from fcf7104 to 138cb80 Compare January 4, 2021 06:11

aschmahmann mentioned this pull request Jan 4, 2021

Add basic crawler #663

Merged

willscott reviewed Jan 4, 2021

View reviewed changes

aschmahmann added 4 commits January 4, 2021 13:57

refactor: rename messageSender to peerMessageSender and added comment…

2c313a8

…s to clarify what peerMessageSender and messageManager do

refactor: factor out reused string

9ebf306

comments: added comments clarifying the contract of getLocal

6fca41f

rename messageManager to messageSenderImpl and renamed dht.messageMgr to

ebd2d69

dht.msgSender

aschmahmann merged commit 03d4b62 into master Jan 4, 2021

aschmahmann mentioned this pull request May 14, 2021

Release v0.9.0 ipfs/kubo#8058

Closed

71 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract DHT message sender from the DHT #659

Extract DHT message sender from the DHT #659

aschmahmann commented Jun 1, 2020

aschmahmann Jun 1, 2020

aarshkshah1992 left a comment

aarshkshah1992 Jun 3, 2020

aschmahmann Jun 3, 2020

aschmahmann Oct 7, 2020

aarshkshah1992 Jun 3, 2020

aschmahmann Jun 3, 2020

aarshkshah1992 Jun 3, 2020

aschmahmann Jun 3, 2020

aschmahmann left a comment

willscott left a comment

willscott Jan 4, 2021

aschmahmann Jan 4, 2021

aschmahmann Jan 4, 2021

willscott Jan 4, 2021

aschmahmann Jan 4, 2021

willscott Jan 4, 2021

aschmahmann Jan 4, 2021

willscott Jan 4, 2021

aschmahmann Jan 4, 2021

willscott Jan 4, 2021

aschmahmann Jan 4, 2021


		import "errors"

		var ErrInvalidRecord = errors.New("received invalid record")

Extract DHT message sender from the DHT #659

Extract DHT message sender from the DHT #659

Conversation

aschmahmann commented Jun 1, 2020

Choose a reason for hiding this comment

aarshkshah1992 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

aschmahmann left a comment

Choose a reason for hiding this comment

willscott 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment