Refactor transport part 1 #324

hannahhoward · 2022-05-19T22:26:47Z

Goals

Well, I got to passing integration tests with the messaging layer underneath the transport. I'm not feeling done at all yet, truthfully. The question now is whether to try to get all tests passing and merge the changeset, or push for something closer to what I think makes sense before we go forward.

The biggest issue at the moment is that the "dtChannel" struct, which grew sort of like a frankenstein to begin with, is even more of a frankenstein now. I think I need to dig into refactoring this. The whole of the transport code is very messy. While I can certainly finish off the unit tests, I don't know what value we get from unit testing some very rough code.

hannahhoward · 2022-05-20T01:52:31Z

Proposed path forward: get unit tests outside the transport passing, comment out transport tests for now, fix transport code in a subsequent PR.

hannahhoward · 2022-05-20T01:53:58Z

So: I'm asking @rvagg and maybe @dirkmc or @willscott to look at the code outside the transport layer with an unfortunate promise for the moment that the code "just works" by way of the integration tests, understanding the merge is to V2, not to master, and there will be subsequence prs before release. (though to be fair there will be additional changes made to the transport boundary for the events handlers.

hannahhoward · 2022-05-20T01:56:12Z

Things I am most interested in review on:

The current transport interface
The "WithTransport" method
Code changes in the impl directory (as opposed to the transport dir)

rvagg · 2022-05-20T04:48:14Z

channels/channel_state.go

 		decoder, _ := c.voucherDecoder(encoded.Type)
 		encodable, _ := decoder.DecodeFromCbor(encoded.Voucher.Raw)
-		vouchers = append(vouchers, encodable.(datatransfer.Voucher))
+		voucher := encodable.(datatransfer.Voucher)
+		c.decodedVouchers[0] = &voucher


Suggested change

c.decodedVouchers[0] = &voucher

c.decodedVouchers[i] = &voucher

looks like a copypasta bug?

rvagg · 2022-05-20T05:14:40Z

impl/restart.go

-	log.Infof("sending open channel to %s to restart channel %s", requestTo, chid)
-	if err := m.transport.OpenChannel(ctx, requestTo, chid, cidlink.Link{Cid: baseCid}, selector, channel, req); err != nil {
+	monitoredChan := m.channelMonitor.AddChannel(chid, channel.IsPull())
+	log.Infof("sending push restart channel to %s for channel %s", requestTo, chid)


Suggested change

log.Infof("sending push restart channel to %s for channel %s", requestTo, chid)

log.Infof("sending restart channel to %s for channel %s", requestTo, chid)

rvagg · 2022-05-20T05:24:29Z

OK, so we're going with the slightly odd WithChannel() actions callback API because different transports may have different "capabilities", so the actions object you get may have different methods on it depending on the transport. I suppose that's reasonable, but some questions to further clarify whether this is really needed:

If you can query transport.Capabilities() prior to even touching WithChannel(), couldn't we just have a single actions object that can be constructed without the need for the actions callback, then pass that in and have it error if you requested actions that the transport doesn't have the capabilities for? i.e. is the actions callback really needed and couldn't it just be done with a declarative config object passed in to WithChannel().
What variations in capabilities are you designing for here? I imagine this is mostly about thinking ahead to the features that bitswap doesn't support that Graphsync does, but maybe you're also thinking broader to other protocols?

hannahhoward · 2022-05-31T04:31:42Z

@rvagg OK I've modified my interfaces significantly, based on your feedback. I still have a kind of "multi-command" in the form of UpdateChannel, but all the individual methods are also here.

Would love your feedback. Also, the transport code should be generally easier to comprehend now.

codecov-commenter · 2022-05-31T04:32:11Z

Codecov Report

❗ No coverage uploaded for pull request base (v2@681bfed). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 6b581f2 differs from pull request most recent head 7fa9bb2. Consider uploading reports for the commit 7fa9bb2 to get more accurate results

@@          Coverage Diff          @@
##             v2     #324   +/-   ##
=====================================
  Coverage      ?   54.77%           
=====================================
  Files         ?       26           
  Lines         ?     2817           
  Branches      ?        0           
=====================================
  Hits          ?     1543           
  Misses        ?     1095           
  Partials      ?      179

rvagg · 2022-05-31T05:48:58Z

impl/impl.go

-	// we aren't able to send the message to the peer because the channel
-	// is already in an error state, which is probably because of connection
-	// issues, so if we cant send the message just log a warning.
+	// Close transfport and try to send a cancel message to the remote peer.


Suggested change

// Close transfport and try to send a cancel message to the remote peer.

// Close transport and try to send a cancel message to the remote peer.

rvagg · 2022-05-31T05:55:11Z

testutil/faketransport.go

@@ -74,22 +129,11 @@ func (ft *FakeTransport) SetEventHandler(events datatransfer.EventsHandler) erro
 	return ft.SetEventHandlerErr
 }

+// Shutdownc loses a this transport


Suggested change

// Shutdownc loses a this transport

// Shutdown loses a this transport

rvagg · 2022-05-31T06:20:58Z

Nice. I haven't quite gone thoroughly through the graphsync bits yet, lots of detail there. But the API is nicer and much more direct.

Two questions:

Could we get rid of PauseChannel(), CloseChannel() and SendMessage() entirely and end up with a simplified API, where you need to go through UpdateChannel() for all of those operations? It might also encourage usage that batches operations if you don't offer the atomic forms.
Is there a case for making ChannelUpdate#SendUpdate a slice instead of a single Message so you could send multiple messages? Or are we not dealing with communication channels that have a case for multiple messages without response?

rvagg · 2022-06-01T02:06:23Z

impl/impl.go

@@ -399,7 +399,10 @@ func (m *manager) CloseDataTransferChannel(ctx context.Context, chid datatransfe
 	))
 	defer span.End()
 	// Close the channel on the local transport
-	err = m.transport.CloseChannel(ctx, chid)
+	err = m.transport.UpdateChannel(ctx, chid, datatransfer.ChannelUpdate{
+		Paused: chst.Status().IsResponderPaused(),


yeah, this is slightly awkward isn't it? as long as we have access to the channel status this then it's probably not the worst thing

you could make it an enum with 3 values: pause, unpause (resume?), continue - where the latter is "just keep on doing what you were doing".

* refactor(transport): move libp2p message layer into transport * refactor(graphsync): cleanup channel state tracking * refactor(dtchannel): extract dtchannel to package * test(impl): get remaining tests passing * refactor(transport): much simpler interface * style(lint): fix imports * refactor(transport): remote unneccesary methods * fix(rebase): fix errors after rebase

hannahhoward changed the base branch from master to v2 May 19, 2022 22:27

hannahhoward requested review from dirkmc, rvagg and willscott May 20, 2022 01:54

rvagg reviewed May 20, 2022

View reviewed changes

hannahhoward force-pushed the feat/refactor-transport-part-1 branch from 27daa3b to ab05cba Compare May 31, 2022 04:28

hannahhoward marked this pull request as ready for review May 31, 2022 04:30

rvagg reviewed May 31, 2022

View reviewed changes

hannahhoward force-pushed the feat/refactor-transport-part-1 branch from b7c9d9e to 1076301 Compare June 1, 2022 01:40

rvagg reviewed Jun 1, 2022

View reviewed changes

hannahhoward added 8 commits June 2, 2022 18:06

refactor(transport): move libp2p message layer into transport

abc76da

refactor(graphsync): cleanup channel state tracking

c9e11e6

refactor(dtchannel): extract dtchannel to package

c8033ed

test(impl): get remaining tests passing

189a811

refactor(transport): much simpler interface

96bbae3

style(lint): fix imports

49260f0

refactor(transport): remote unneccesary methods

362adea

fix(rebase): fix errors after rebase

7fa9bb2

hannahhoward force-pushed the feat/refactor-transport-part-1 branch from 1076301 to 7fa9bb2 Compare June 3, 2022 01:19

hannahhoward force-pushed the feat/refactor-transport-part-1 branch from 6b581f2 to 7fa9bb2 Compare June 16, 2022 02:59

hannahhoward changed the base branch from v2 to feat/refactor-transport June 16, 2022 03:20

hannahhoward merged commit 51da526 into feat/refactor-transport Jun 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor transport part 1 #324

Refactor transport part 1 #324

hannahhoward commented May 19, 2022

hannahhoward commented May 20, 2022

hannahhoward commented May 20, 2022

hannahhoward commented May 20, 2022

rvagg May 20, 2022

rvagg May 20, 2022

rvagg commented May 20, 2022

hannahhoward commented May 31, 2022

codecov-commenter commented May 31, 2022 •

edited

Loading

rvagg May 31, 2022

rvagg May 31, 2022

rvagg commented May 31, 2022

rvagg Jun 1, 2022

	c.decodedVouchers[0] = &voucher
	c.decodedVouchers[i] = &voucher

	log.Infof("sending push restart channel to %s for channel %s", requestTo, chid)
	log.Infof("sending restart channel to %s for channel %s", requestTo, chid)

	// Close transfport and try to send a cancel message to the remote peer.
	// Close transport and try to send a cancel message to the remote peer.

	// Shutdownc loses a this transport
	// Shutdown loses a this transport

Refactor transport part 1 #324

Refactor transport part 1 #324

Conversation

hannahhoward commented May 19, 2022

Goals

hannahhoward commented May 20, 2022

hannahhoward commented May 20, 2022

hannahhoward commented May 20, 2022

rvagg May 20, 2022

Choose a reason for hiding this comment

rvagg May 20, 2022

Choose a reason for hiding this comment

rvagg commented May 20, 2022

hannahhoward commented May 31, 2022

codecov-commenter commented May 31, 2022 • edited Loading

Codecov Report

rvagg May 31, 2022

Choose a reason for hiding this comment

rvagg May 31, 2022

Choose a reason for hiding this comment

rvagg commented May 31, 2022

rvagg Jun 1, 2022

Choose a reason for hiding this comment

codecov-commenter commented May 31, 2022 •

edited

Loading