From 4c59e3b1cb3d4e1ee6f4679274a0f8705f0acae3 Mon Sep 17 00:00:00 2001 From: hannahhoward Date: Mon, 13 Dec 2021 17:58:01 -0800 Subject: [PATCH 01/12] feat(message): add scrub response method to builder --- message/builder.go | 21 +++ message/builder_test.go | 330 ++++++++++++++++++++++------------------ 2 files changed, 200 insertions(+), 151 deletions(-) diff --git a/message/builder.go b/message/builder.go index 30df4453..da0d497e 100644 --- a/message/builder.go +++ b/message/builder.go @@ -87,6 +87,27 @@ func (b *Builder) Empty() bool { return len(b.requests) == 0 && len(b.outgoingBlocks) == 0 && len(b.outgoingResponses) == 0 } +// ScrubResponse removes a response from a message and any blocks only referenced by that response +func (b *Builder) ScrubResponse(requestID graphsync.RequestID) { + delete(b.completedResponses, requestID) + delete(b.extensions, requestID) + delete(b.outgoingResponses, requestID) + newBlkSize := uint64(0) + savedBlocks := make(map[cid.Cid]blocks.Block, len(b.outgoingBlocks)) + for _, metadata := range b.outgoingResponses { + for _, item := range metadata { + block, willSendBlock := b.outgoingBlocks[item.Link] + _, alreadySavedBlock := savedBlocks[item.Link] + if item.BlockPresent && willSendBlock && !alreadySavedBlock { + savedBlocks[item.Link] = block + newBlkSize += uint64(len(block.RawData())) + } + } + } + b.blkSize = newBlkSize + b.outgoingBlocks = savedBlocks +} + // Build assembles and encodes message data from the added requests, links, and blocks. func (b *Builder) Build() (GraphSyncMessage, error) { responses := make(map[graphsync.RequestID]GraphSyncResponse, len(b.outgoingResponses)) diff --git a/message/builder_test.go b/message/builder_test.go index 0464d7d1..46b2df85 100644 --- a/message/builder_test.go +++ b/message/builder_test.go @@ -1,7 +1,6 @@ package message import ( - "fmt" "math/rand" "testing" @@ -15,40 +14,11 @@ import ( ) func TestMessageBuilding(t *testing.T) { - rb := NewBuilder(Topic(0)) blocks := testutil.GenerateBlocksOfSize(3, 100) links := make([]ipld.Link, 0, len(blocks)) for _, block := range blocks { links = append(links, cidlink.Link{Cid: block.Cid()}) } - requestID1 := graphsync.RequestID(rand.Int31()) - requestID2 := graphsync.RequestID(rand.Int31()) - requestID3 := graphsync.RequestID(rand.Int31()) - requestID4 := graphsync.RequestID(rand.Int31()) - - rb.AddLink(requestID1, links[0], true) - rb.AddLink(requestID1, links[1], false) - rb.AddLink(requestID1, links[2], true) - - rb.AddResponseCode(requestID1, graphsync.RequestCompletedPartial) - - rb.AddLink(requestID2, links[1], true) - rb.AddLink(requestID2, links[2], true) - rb.AddLink(requestID2, links[1], true) - - rb.AddResponseCode(requestID2, graphsync.RequestCompletedFull) - - rb.AddLink(requestID3, links[0], true) - rb.AddLink(requestID3, links[1], true) - - rb.AddResponseCode(requestID4, graphsync.RequestCompletedFull) - - for _, block := range blocks { - rb.AddBlock(block) - } - - require.Equal(t, uint64(300), rb.BlockSize(), "did not calculate block size correctly") - extensionData1 := testutil.RandomBytes(100) extensionName1 := graphsync.ExtensionName("AppleSauce/McGee") extension1 := graphsync.ExtensionData{ @@ -61,136 +31,194 @@ func TestMessageBuilding(t *testing.T) { Name: extensionName2, Data: extensionData2, } - rb.AddExtensionData(requestID1, extension1) - rb.AddExtensionData(requestID3, extension2) - - message, err := rb.Build() - - require.NoError(t, err, "build responses errored") - responses := message.Responses() - sentBlocks := message.Blocks() - require.Len(t, responses, 4, "did not assemble correct number of responses") - - response1, err := findResponseForRequestID(responses, requestID1) - require.NoError(t, err) - require.Equal(t, graphsync.RequestCompletedPartial, response1.Status(), "did not generate completed partial response") - - response1MetadataRaw, found := response1.Extension(graphsync.ExtensionMetadata) - require.True(t, found, "Metadata should be included in response") - response1Metadata, err := metadata.DecodeMetadata(response1MetadataRaw) - require.NoError(t, err) - require.Equal(t, response1Metadata, metadata.Metadata{ - metadata.Item{Link: links[0].(cidlink.Link).Cid, BlockPresent: true}, - metadata.Item{Link: links[1].(cidlink.Link).Cid, BlockPresent: false}, - metadata.Item{Link: links[2].(cidlink.Link).Cid, BlockPresent: true}, - }, "incorrect metadata included in response") - - response1ReturnedExtensionData, found := response1.Extension(extensionName1) - require.True(t, found) - require.Equal(t, extensionData1, response1ReturnedExtensionData, "did not encode first extension") - - response2, err := findResponseForRequestID(responses, requestID2) - require.NoError(t, err) - require.Equal(t, graphsync.RequestCompletedFull, response2.Status(), "did not generate completed full response") - - response2MetadataRaw, found := response2.Extension(graphsync.ExtensionMetadata) - require.True(t, found, "Metadata should be included in response") - response2Metadata, err := metadata.DecodeMetadata(response2MetadataRaw) - require.NoError(t, err) - require.Equal(t, response2Metadata, metadata.Metadata{ - metadata.Item{Link: links[1].(cidlink.Link).Cid, BlockPresent: true}, - metadata.Item{Link: links[2].(cidlink.Link).Cid, BlockPresent: true}, - metadata.Item{Link: links[1].(cidlink.Link).Cid, BlockPresent: true}, - }, "incorrect metadata included in response") - - response3, err := findResponseForRequestID(responses, requestID3) - require.NoError(t, err) - require.Equal(t, graphsync.PartialResponse, response3.Status(), "did not generate partial response") - - response3MetadataRaw, found := response3.Extension(graphsync.ExtensionMetadata) - require.True(t, found, "Metadata should be included in response") - response3Metadata, err := metadata.DecodeMetadata(response3MetadataRaw) - require.NoError(t, err) - require.Equal(t, response3Metadata, metadata.Metadata{ - metadata.Item{Link: links[0].(cidlink.Link).Cid, BlockPresent: true}, - metadata.Item{Link: links[1].(cidlink.Link).Cid, BlockPresent: true}, - }, "incorrect metadata included in response") - - response3ReturnedExtensionData, found := response3.Extension(extensionName2) - require.True(t, found) - require.Equal(t, extensionData2, response3ReturnedExtensionData, "did not encode second extension") - - response4, err := findResponseForRequestID(responses, requestID4) - require.NoError(t, err) - require.Equal(t, graphsync.RequestCompletedFull, response4.Status(), "did not generate completed full response") - - require.Equal(t, len(blocks), len(sentBlocks), "did not send all blocks") - - for _, block := range sentBlocks { - testutil.AssertContainsBlock(t, blocks, block) - } -} - -func TestMessageBuildingExtensionOnly(t *testing.T) { - rb := NewBuilder(Topic(0)) requestID1 := graphsync.RequestID(rand.Int31()) requestID2 := graphsync.RequestID(rand.Int31()) + requestID3 := graphsync.RequestID(rand.Int31()) + requestID4 := graphsync.RequestID(rand.Int31()) - extensionData1 := testutil.RandomBytes(100) - extensionName1 := graphsync.ExtensionName("AppleSauce/McGee") - extension1 := graphsync.ExtensionData{ - Name: extensionName1, - Data: extensionData1, + testCases := map[string]struct { + build func(*Builder) + expectedSize uint64 + checkMsg func(t *testing.T, message GraphSyncMessage) + }{ + "normal building": { + build: func(rb *Builder) { + + rb.AddLink(requestID1, links[0], true) + rb.AddLink(requestID1, links[1], false) + rb.AddLink(requestID1, links[2], true) + + rb.AddResponseCode(requestID1, graphsync.RequestCompletedPartial) + + rb.AddLink(requestID2, links[1], true) + rb.AddLink(requestID2, links[2], true) + rb.AddLink(requestID2, links[1], true) + + rb.AddResponseCode(requestID2, graphsync.RequestCompletedFull) + + rb.AddLink(requestID3, links[0], true) + rb.AddLink(requestID3, links[1], true) + + rb.AddResponseCode(requestID4, graphsync.RequestCompletedFull) + rb.AddExtensionData(requestID1, extension1) + rb.AddExtensionData(requestID3, extension2) + for _, block := range blocks { + rb.AddBlock(block) + } + }, + expectedSize: 300, + checkMsg: func(t *testing.T, message GraphSyncMessage) { + + responses := message.Responses() + sentBlocks := message.Blocks() + require.Len(t, responses, 4, "did not assemble correct number of responses") + + response1 := findResponseForRequestID(t, responses, requestID1) + require.Equal(t, graphsync.RequestCompletedPartial, response1.Status(), "did not generate completed partial response") + assertMetadata(t, response1, metadata.Metadata{ + metadata.Item{Link: links[0].(cidlink.Link).Cid, BlockPresent: true}, + metadata.Item{Link: links[1].(cidlink.Link).Cid, BlockPresent: false}, + metadata.Item{Link: links[2].(cidlink.Link).Cid, BlockPresent: true}, + }) + assertExtension(t, response1, extension1) + + response2 := findResponseForRequestID(t, responses, requestID2) + require.Equal(t, graphsync.RequestCompletedFull, response2.Status(), "did not generate completed full response") + assertMetadata(t, response2, metadata.Metadata{ + metadata.Item{Link: links[1].(cidlink.Link).Cid, BlockPresent: true}, + metadata.Item{Link: links[2].(cidlink.Link).Cid, BlockPresent: true}, + metadata.Item{Link: links[1].(cidlink.Link).Cid, BlockPresent: true}, + }) + + response3 := findResponseForRequestID(t, responses, requestID3) + require.Equal(t, graphsync.PartialResponse, response3.Status(), "did not generate partial response") + assertMetadata(t, response3, metadata.Metadata{ + metadata.Item{Link: links[0].(cidlink.Link).Cid, BlockPresent: true}, + metadata.Item{Link: links[1].(cidlink.Link).Cid, BlockPresent: true}, + }) + assertExtension(t, response3, extension2) + + response4 := findResponseForRequestID(t, responses, requestID4) + require.Equal(t, graphsync.RequestCompletedFull, response4.Status(), "did not generate completed full response") + + require.Equal(t, len(blocks), len(sentBlocks), "did not send all blocks") + + for _, block := range sentBlocks { + testutil.AssertContainsBlock(t, blocks, block) + } + }, + }, + "message with only extensions": { + build: func(rb *Builder) { + rb.AddExtensionData(requestID1, extension1) + rb.AddExtensionData(requestID2, extension2) + }, + expectedSize: 0, + checkMsg: func(t *testing.T, message GraphSyncMessage) { + responses := message.Responses() + + response1 := findResponseForRequestID(t, responses, requestID1) + require.Equal(t, graphsync.PartialResponse, response1.Status(), "did not generate partial response") + assertMetadata(t, response1, nil) + assertExtension(t, response1, extension1) + + response2 := findResponseForRequestID(t, responses, requestID2) + require.Equal(t, graphsync.PartialResponse, response2.Status(), "did not generate partial response") + assertMetadata(t, response2, nil) + assertExtension(t, response2, extension2) + }, + }, + "scrub response": { + build: func(rb *Builder) { + + rb.AddLink(requestID1, links[0], true) + rb.AddLink(requestID1, links[1], false) + rb.AddLink(requestID1, links[2], true) + + rb.AddResponseCode(requestID1, graphsync.RequestCompletedPartial) + + rb.AddLink(requestID2, links[1], true) + rb.AddLink(requestID2, links[2], true) + rb.AddLink(requestID2, links[1], true) + + rb.AddResponseCode(requestID2, graphsync.RequestCompletedFull) + + rb.AddLink(requestID3, links[1], true) + + rb.AddResponseCode(requestID4, graphsync.RequestCompletedFull) + rb.AddExtensionData(requestID1, extension1) + rb.AddExtensionData(requestID3, extension2) + for _, block := range blocks { + rb.AddBlock(block) + } + rb.ScrubResponse(requestID1) + }, + expectedSize: 200, + checkMsg: func(t *testing.T, message GraphSyncMessage) { + + responses := message.Responses() + sentBlocks := message.Blocks() + require.Len(t, responses, 3, "did not assemble correct number of responses") + + response2 := findResponseForRequestID(t, responses, requestID2) + require.Equal(t, graphsync.RequestCompletedFull, response2.Status(), "did not generate completed full response") + assertMetadata(t, response2, metadata.Metadata{ + metadata.Item{Link: links[1].(cidlink.Link).Cid, BlockPresent: true}, + metadata.Item{Link: links[2].(cidlink.Link).Cid, BlockPresent: true}, + metadata.Item{Link: links[1].(cidlink.Link).Cid, BlockPresent: true}, + }) + + response3 := findResponseForRequestID(t, responses, requestID3) + require.Equal(t, graphsync.PartialResponse, response3.Status(), "did not generate partial response") + assertMetadata(t, response3, metadata.Metadata{ + metadata.Item{Link: links[1].(cidlink.Link).Cid, BlockPresent: true}, + }) + assertExtension(t, response3, extension2) + + response4 := findResponseForRequestID(t, responses, requestID4) + require.Equal(t, graphsync.RequestCompletedFull, response4.Status(), "did not generate completed full response") + + require.Equal(t, len(blocks)-1, len(sentBlocks), "did not send all blocks") + + testutil.AssertContainsBlock(t, sentBlocks, blocks[1]) + testutil.AssertContainsBlock(t, sentBlocks, blocks[2]) + testutil.RefuteContainsBlock(t, sentBlocks, blocks[0]) + + }, + }, } - extensionData2 := testutil.RandomBytes(100) - extensionName2 := graphsync.ExtensionName("HappyLand/Happenstance") - extension2 := graphsync.ExtensionData{ - Name: extensionName2, - Data: extensionData2, + for testCase, data := range testCases { + t.Run(testCase, func(t *testing.T) { + b := NewBuilder(Topic(rand.Uint64())) + data.build(b) + require.Equal(t, data.expectedSize, b.BlockSize(), "did not calculate block size correctly") + message, err := b.Build() + require.NoError(t, err, "build message errored") + data.checkMsg(t, message) + }) } - rb.AddExtensionData(requestID1, extension1) - rb.AddExtensionData(requestID2, extension2) - - message, err := rb.Build() - - require.NoError(t, err, "build responses errored") - responses := message.Responses() - - response1, err := findResponseForRequestID(responses, requestID1) - require.NoError(t, err) - require.Equal(t, graphsync.PartialResponse, response1.Status(), "did not generate partial response") - - response1MetadataRaw, found := response1.Extension(graphsync.ExtensionMetadata) - require.True(t, found, "Metadata should be included in response") - response1Metadata, err := metadata.DecodeMetadata(response1MetadataRaw) - require.NoError(t, err) - require.Nil(t, response1Metadata, "incorrect metadata included in response") - - response1ReturnedExtensionData, found := response1.Extension(extensionName1) - require.True(t, found) - require.Equal(t, extensionData1, response1ReturnedExtensionData, "did not encode first extension") - - response2, err := findResponseForRequestID(responses, requestID2) - require.NoError(t, err) - require.Equal(t, graphsync.PartialResponse, response2.Status(), "did not generate partial response") - - response2MetadataRaw, found := response2.Extension(graphsync.ExtensionMetadata) - require.True(t, found, "Metadata should be included in response") - response2Metadata, err := metadata.DecodeMetadata(response2MetadataRaw) - require.NoError(t, err) - require.Nil(t, response2Metadata, "incorrect metadata included in response") - - response2ReturnedExtensionData, found := response2.Extension(extensionName2) - require.True(t, found) - require.Equal(t, extensionData2, response2ReturnedExtensionData, "did not encode second extension") - } -func findResponseForRequestID(responses []GraphSyncResponse, requestID graphsync.RequestID) (GraphSyncResponse, error) { +func findResponseForRequestID(t *testing.T, responses []GraphSyncResponse, requestID graphsync.RequestID) GraphSyncResponse { for _, response := range responses { if response.RequestID() == requestID { - return response, nil + return response } } - return GraphSyncResponse{}, fmt.Errorf("Response Not Found") + require.FailNow(t, "Could not find request") + return GraphSyncResponse{} +} + +func assertExtension(t *testing.T, response GraphSyncResponse, extension graphsync.ExtensionData) { + returnedExtensionData, found := response.Extension(extension.Name) + require.True(t, found) + require.Equal(t, extension.Data, returnedExtensionData, "did not encode extension") +} + +func assertMetadata(t *testing.T, response GraphSyncResponse, expectedMetadata metadata.Metadata) { + responseMetadataRaw, found := response.Extension(graphsync.ExtensionMetadata) + require.True(t, found, "Metadata should be included in response") + responseMetadata, err := metadata.DecodeMetadata(responseMetadataRaw) + require.NoError(t, err) + require.Equal(t, expectedMetadata, responseMetadata, "incorrect metadata included in response") } From b4bb8054e76bcf4aca56602f95a0207eaf722ae8 Mon Sep 17 00:00:00 2001 From: hannahhoward Date: Tue, 14 Dec 2021 20:46:40 -0800 Subject: [PATCH 02/12] feat(messagequeue): add stream closer concept --- message/builder.go | 27 ++++- message/builder_test.go | 65 +++++++++++- messagequeue/messagequeue.go | 117 +++++++++++++++------ messagequeue/messagequeue_test.go | 168 +++++++++++++++++++++++------- 4 files changed, 300 insertions(+), 77 deletions(-) diff --git a/message/builder.go b/message/builder.go index da0d497e..6914f08a 100644 --- a/message/builder.go +++ b/message/builder.go @@ -1,6 +1,8 @@ package message import ( + "io" + blocks "github.com/ipfs/go-block-format" "github.com/ipfs/go-cid" "github.com/ipld/go-ipld-prime" @@ -21,6 +23,7 @@ type Builder struct { outgoingResponses map[graphsync.RequestID]metadata.Metadata extensions map[graphsync.RequestID][]graphsync.ExtensionData requests map[graphsync.RequestID]GraphSyncRequest + responseStreams map[graphsync.RequestID]io.Closer } // Topic is an identifier for notifications about this response builder @@ -35,9 +38,20 @@ func NewBuilder(topic Topic) *Builder { completedResponses: make(map[graphsync.RequestID]graphsync.ResponseStatusCode), outgoingResponses: make(map[graphsync.RequestID]metadata.Metadata), extensions: make(map[graphsync.RequestID][]graphsync.ExtensionData), + responseStreams: make(map[graphsync.RequestID]io.Closer), } } +// AddResponseStream adds a stream that can be closed if the message fails to send +func (b *Builder) AddResponseStream(requestID graphsync.RequestID, responseStream io.Closer) { + b.responseStreams[requestID] = responseStream +} + +// ResponseStreams returns related response streams for a given builder +func (b *Builder) ResponseStreams() map[graphsync.RequestID]io.Closer { + return b.responseStreams +} + // AddRequest registers a new request to be added to the message. func (b *Builder) AddRequest(request GraphSyncRequest) { b.requests[request.ID()] = request @@ -88,10 +102,14 @@ func (b *Builder) Empty() bool { } // ScrubResponse removes a response from a message and any blocks only referenced by that response -func (b *Builder) ScrubResponse(requestID graphsync.RequestID) { - delete(b.completedResponses, requestID) - delete(b.extensions, requestID) - delete(b.outgoingResponses, requestID) +func (b *Builder) ScrubResponses(requestIDs []graphsync.RequestID) uint64 { + for _, requestID := range requestIDs { + delete(b.completedResponses, requestID) + delete(b.extensions, requestID) + delete(b.outgoingResponses, requestID) + delete(b.responseStreams, requestID) + } + oldSize := b.blkSize newBlkSize := uint64(0) savedBlocks := make(map[cid.Cid]blocks.Block, len(b.outgoingBlocks)) for _, metadata := range b.outgoingResponses { @@ -106,6 +124,7 @@ func (b *Builder) ScrubResponse(requestID graphsync.RequestID) { } b.blkSize = newBlkSize b.outgoingBlocks = savedBlocks + return oldSize - newBlkSize } // Build assembles and encodes message data from the added requests, links, and blocks. diff --git a/message/builder_test.go b/message/builder_test.go index 46b2df85..2995636b 100644 --- a/message/builder_test.go +++ b/message/builder_test.go @@ -1,6 +1,7 @@ package message import ( + "io" "math/rand" "testing" @@ -35,11 +36,12 @@ func TestMessageBuilding(t *testing.T) { requestID2 := graphsync.RequestID(rand.Int31()) requestID3 := graphsync.RequestID(rand.Int31()) requestID4 := graphsync.RequestID(rand.Int31()) - + closer := io.NopCloser(nil) testCases := map[string]struct { - build func(*Builder) - expectedSize uint64 - checkMsg func(t *testing.T, message GraphSyncMessage) + build func(*Builder) + expectedSize uint64 + expectedStreams map[graphsync.RequestID]io.Closer + checkMsg func(t *testing.T, message GraphSyncMessage) }{ "normal building": { build: func(rb *Builder) { @@ -65,8 +67,14 @@ func TestMessageBuilding(t *testing.T) { for _, block := range blocks { rb.AddBlock(block) } + rb.AddResponseStream(requestID1, closer) + rb.AddResponseStream(requestID2, closer) }, expectedSize: 300, + expectedStreams: map[graphsync.RequestID]io.Closer{ + requestID1: closer, + requestID2: closer, + }, checkMsg: func(t *testing.T, message GraphSyncMessage) { responses := message.Responses() @@ -151,7 +159,7 @@ func TestMessageBuilding(t *testing.T) { for _, block := range blocks { rb.AddBlock(block) } - rb.ScrubResponse(requestID1) + rb.ScrubResponses([]graphsync.RequestID{requestID1}) }, expectedSize: 200, checkMsg: func(t *testing.T, message GraphSyncMessage) { @@ -186,12 +194,59 @@ func TestMessageBuilding(t *testing.T) { }, }, + "scrub multiple responses": { + build: func(rb *Builder) { + + rb.AddLink(requestID1, links[0], true) + rb.AddLink(requestID1, links[1], false) + rb.AddLink(requestID1, links[2], true) + + rb.AddResponseCode(requestID1, graphsync.RequestCompletedPartial) + + rb.AddLink(requestID2, links[1], true) + rb.AddLink(requestID2, links[2], true) + rb.AddLink(requestID2, links[1], true) + + rb.AddResponseCode(requestID2, graphsync.RequestCompletedFull) + + rb.AddLink(requestID3, links[1], true) + + rb.AddResponseCode(requestID4, graphsync.RequestCompletedFull) + rb.AddExtensionData(requestID1, extension1) + rb.AddExtensionData(requestID3, extension2) + for _, block := range blocks { + rb.AddBlock(block) + } + rb.ScrubResponses([]graphsync.RequestID{requestID1, requestID2, requestID4}) + }, + expectedSize: 100, + checkMsg: func(t *testing.T, message GraphSyncMessage) { + + responses := message.Responses() + sentBlocks := message.Blocks() + require.Len(t, responses, 1, "did not assemble correct number of responses") + + response3 := findResponseForRequestID(t, responses, requestID3) + require.Equal(t, graphsync.PartialResponse, response3.Status(), "did not generate partial response") + assertMetadata(t, response3, metadata.Metadata{ + metadata.Item{Link: links[1].(cidlink.Link).Cid, BlockPresent: true}, + }) + assertExtension(t, response3, extension2) + + testutil.AssertContainsBlock(t, sentBlocks, blocks[1]) + testutil.RefuteContainsBlock(t, sentBlocks, blocks[2]) + testutil.RefuteContainsBlock(t, sentBlocks, blocks[0]) + }, + }, } for testCase, data := range testCases { t.Run(testCase, func(t *testing.T) { b := NewBuilder(Topic(rand.Uint64())) data.build(b) require.Equal(t, data.expectedSize, b.BlockSize(), "did not calculate block size correctly") + if data.expectedStreams != nil { + require.Equal(t, data.expectedStreams, b.ResponseStreams()) + } message, err := b.Build() require.NoError(t, err, "build message errored") data.checkMsg(t, message) diff --git a/messagequeue/messagequeue.go b/messagequeue/messagequeue.go index bd7df40b..f7cfb4e4 100644 --- a/messagequeue/messagequeue.go +++ b/messagequeue/messagequeue.go @@ -4,9 +4,11 @@ import ( "context" "errors" "fmt" + "io" "sync" "time" + "github.com/ipfs/go-graphsync" logging "github.com/ipfs/go-log/v2" "github.com/libp2p/go-libp2p-core/peer" @@ -96,6 +98,24 @@ func (mq *MessageQueue) AllocateAndBuildMessage(size uint64, buildMessageFn func } } +type responseStream struct { + requestID graphsync.RequestID + closed bool + closedLk sync.RWMutex +} + +func (r *responseStream) close() { + r.closedLk.Lock() + r.closed = true + r.closedLk.Unlock() +} + +func (r *responseStream) isClosed() bool { + r.closedLk.RLock() + defer r.closedLk.RUnlock() + return r.closed +} + func (mq *MessageQueue) buildMessage(size uint64, buildMessageFn func(*gsmsg.Builder), notifees []notifications.Notifee) bool { mq.buildersLk.Lock() defer mq.buildersLk.Unlock() @@ -147,10 +167,10 @@ func (mq *MessageQueue) runQueue() { select { case <-mq.outgoingWork: for { - _, publisher, err := mq.extractOutgoingMessage() + _, metadata, err := mq.extractOutgoingMessage() if err == nil { - publisher.publishError(fmt.Errorf("message queue shutdown")) - publisher.close() + mq.publishError(metadata, fmt.Errorf("message queue shutdown")) + mq.eventPublisher.Close(metadata.topic) } else { break } @@ -179,12 +199,12 @@ func (mq *MessageQueue) signalWork() { var errEmptyMessage = errors.New("empty Message") -func (mq *MessageQueue) extractOutgoingMessage() (gsmsg.GraphSyncMessage, *messagePublisher, error) { +func (mq *MessageQueue) extractOutgoingMessage() (gsmsg.GraphSyncMessage, messageMetadata, error) { // grab outgoing message mq.buildersLk.Lock() if len(mq.builders) == 0 { mq.buildersLk.Unlock() - return gsmsg.GraphSyncMessage{}, nil, errEmptyMessage + return gsmsg.GraphSyncMessage{}, messageMetadata{}, errEmptyMessage } builder := mq.builders[0] mq.builders = mq.builders[1:] @@ -197,37 +217,69 @@ func (mq *MessageQueue) extractOutgoingMessage() (gsmsg.GraphSyncMessage, *messa } mq.buildersLk.Unlock() if builder.Empty() { - return gsmsg.GraphSyncMessage{}, nil, errEmptyMessage + return gsmsg.GraphSyncMessage{}, messageMetadata{}, errEmptyMessage } message, err := builder.Build() - return message, &messagePublisher{mq, builder.Topic(), builder.BlockSize()}, err + return message, messageMetadata{builder.Topic(), builder.ResponseStreams(), builder.BlockSize()}, err } func (mq *MessageQueue) sendMessage() { - message, publisher, err := mq.extractOutgoingMessage() + message, metadata, err := mq.extractOutgoingMessage() if err != nil { if err != errEmptyMessage { log.Errorf("Unable to assemble GraphSync message: %s", err.Error()) } return } - publisher.publishQueued() - defer publisher.close() + mq.publishQueued(metadata) + defer mq.eventPublisher.Close(metadata.topic) err = mq.initializeSender() if err != nil { log.Infof("cant open message sender to peer %s: %s", mq.p, err) // TODO: cant connect, what now? - publisher.publishError(fmt.Errorf("cant open message sender to peer %s: %w", mq.p, err)) + mq.publishError(metadata, fmt.Errorf("cant open message sender to peer %s: %w", mq.p, err)) return } for i := 0; i < mq.maxRetries; i++ { // try to send this message until we fail. - if mq.attemptSendAndRecovery(message, publisher) { + if mq.attemptSendAndRecovery(message, metadata) { return } } - publisher.publishError(fmt.Errorf("expended retries on SendMsg(%s)", mq.p)) + mq.publishError(metadata, fmt.Errorf("expended retries on SendMsg(%s)", mq.p)) +} + +func (mq *MessageQueue) scrubResponseStreams(responseStreams map[graphsync.RequestID]io.Closer) { + requestIDs := make([]graphsync.RequestID, 0, len(responseStreams)) + for requestID, responseStream := range responseStreams { + _ = responseStream.Close() + requestIDs = append(requestIDs, requestID) + } + totalFreed := mq.scrubResponses(requestIDs) + if totalFreed > 0 { + err := mq.allocator.ReleaseBlockMemory(mq.p, totalFreed) + if err != nil { + log.Error(err) + } + } +} + +// ScrubResponses removes the given response and associated blocks +// from all pending messages in the queue +func (mq *MessageQueue) scrubResponses(requestIDs []graphsync.RequestID) uint64 { + mq.buildersLk.Lock() + newBuilders := make([]*gsmsg.Builder, 0, len(mq.builders)) + totalFreed := uint64(0) + for _, builder := range mq.builders { + totalFreed = builder.ScrubResponses(requestIDs) + if !builder.Empty() { + newBuilders = append(newBuilders, builder) + } + } + mq.builders = newBuilders + mq.buildersLk.Unlock() + return totalFreed } func (mq *MessageQueue) initializeSender() error { @@ -242,10 +294,10 @@ func (mq *MessageQueue) initializeSender() error { return nil } -func (mq *MessageQueue) attemptSendAndRecovery(message gsmsg.GraphSyncMessage, publisher *messagePublisher) bool { +func (mq *MessageQueue) attemptSendAndRecovery(message gsmsg.GraphSyncMessage, metadata messageMetadata) bool { err := mq.sender.SendMsg(mq.ctx, message) if err == nil { - publisher.publishSent() + mq.publishSent(metadata) return true } @@ -255,10 +307,10 @@ func (mq *MessageQueue) attemptSendAndRecovery(message gsmsg.GraphSyncMessage, p select { case <-mq.done: - publisher.publishError(errors.New("queue shutdown")) + mq.publishError(metadata, errors.New("queue shutdown")) return true case <-mq.ctx.Done(): - publisher.publishError(errors.New("context cancelled")) + mq.publishError(metadata, errors.New("context cancelled")) return true case <-time.After(time.Millisecond * 100): // wait 100ms in case disconnect notifications are still propogating @@ -272,7 +324,7 @@ func (mq *MessageQueue) attemptSendAndRecovery(message gsmsg.GraphSyncMessage, p // I think the *right* answer is to probably put the message we're // trying to send back, and then return to waiting for new work or // a disconnect. - publisher.publishError(fmt.Errorf("couldnt open sender again after SendMsg(%s) failed: %w", mq.p, err)) + mq.publishError(metadata, fmt.Errorf("couldnt open sender again after SendMsg(%s) failed: %w", mq.p, err)) return true } @@ -298,26 +350,27 @@ func openSender(ctx context.Context, network MessageNetwork, p peer.ID, sendTime return nsender, nil } -type messagePublisher struct { - mq *MessageQueue - topic gsmsg.Topic - msgSize uint64 +type messageMetadata struct { + topic gsmsg.Topic + responseStreams map[graphsync.RequestID]io.Closer + msgSize uint64 } -func (mp *messagePublisher) publishQueued() { - mp.mq.eventPublisher.Publish(mp.topic, Event{Name: Queued}) +func (mq *MessageQueue) publishQueued(metadata messageMetadata) { + mq.eventPublisher.Publish(metadata.topic, Event{Name: Queued}) } -func (mp *messagePublisher) publishSent() { - mp.mq.eventPublisher.Publish(mp.topic, Event{Name: Sent}) - _ = mp.mq.allocator.ReleaseBlockMemory(mp.mq.p, mp.msgSize) +func (mq *MessageQueue) publishSent(metadata messageMetadata) { + mq.eventPublisher.Publish(metadata.topic, Event{Name: Sent}) + _ = mq.allocator.ReleaseBlockMemory(mq.p, metadata.msgSize) } -func (mp *messagePublisher) publishError(err error) { - mp.mq.eventPublisher.Publish(mp.topic, Event{Name: Error, Err: err}) - _ = mp.mq.allocator.ReleaseBlockMemory(mp.mq.p, mp.msgSize) +func (mq *MessageQueue) publishError(metadata messageMetadata, err error) { + mq.scrubResponseStreams(metadata.responseStreams) + mq.eventPublisher.Publish(metadata.topic, Event{Name: Error, Err: err}) + _ = mq.allocator.ReleaseBlockMemory(mq.p, metadata.msgSize) } -func (mp *messagePublisher) close() { - mp.mq.eventPublisher.Close(mp.topic) +func (mq *MessageQueue) close(metadata messageMetadata) { + mq.eventPublisher.Close(metadata.topic) } diff --git a/messagequeue/messagequeue_test.go b/messagequeue/messagequeue_test.go index 9e8c79a5..a991b721 100644 --- a/messagequeue/messagequeue_test.go +++ b/messagequeue/messagequeue_test.go @@ -2,12 +2,14 @@ package messagequeue import ( "context" + "errors" "fmt" "math/rand" "sync" "testing" "time" + cidlink "github.com/ipld/go-ipld-prime/linking/cid" "github.com/ipld/go-ipld-prime/node/basicnode" "github.com/ipld/go-ipld-prime/traversal/selector/builder" "github.com/libp2p/go-libp2p-core/peer" @@ -21,42 +23,6 @@ import ( "github.com/ipfs/go-graphsync/testutil" ) -const sendMessageTimeout = 10 * time.Minute -const messageSendRetries = 10 - -type fakeMessageNetwork struct { - connectError error - messageSenderError error - messageSender gsnet.MessageSender - wait *sync.WaitGroup -} - -func (fmn *fakeMessageNetwork) ConnectTo(context.Context, peer.ID) error { - return fmn.connectError -} - -func (fmn *fakeMessageNetwork) NewMessageSender(context.Context, peer.ID, gsnet.MessageSenderOpts) (gsnet.MessageSender, error) { - fmn.wait.Done() - if fmn.messageSenderError == nil { - return fmn.messageSender, nil - } - return nil, fmn.messageSenderError -} - -type fakeMessageSender struct { - sendError error - fullClosed chan<- struct{} - reset chan<- struct{} - messagesSent chan<- gsmsg.GraphSyncMessage -} - -func (fms *fakeMessageSender) SendMsg(ctx context.Context, msg gsmsg.GraphSyncMessage) error { - fms.messagesSent <- msg - return fms.sendError -} -func (fms *fakeMessageSender) Close() error { fms.fullClosed <- struct{}{}; return nil } -func (fms *fakeMessageSender) Reset() error { fms.reset <- struct{}{}; return nil } - func TestStartupAndShutdown(t *testing.T) { ctx := context.Background() ctx, cancel := context.WithTimeout(ctx, 1*time.Second) @@ -386,3 +352,133 @@ func TestSendsResponsesMemoryPressure(t *testing.T) { t.Fatal("timeout waiting for transaction to complete") } } + +func TestNetworkErrorClearResponses(t *testing.T) { + ctx := context.Background() + ctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + + peer := testutil.GeneratePeers(1)[0] + messagesSent := make(chan gsmsg.GraphSyncMessage) + resetChan := make(chan struct{}, 1) + fullClosedChan := make(chan struct{}, 1) + requestID1 := graphsync.RequestID(rand.Int31()) + requestID2 := graphsync.RequestID(rand.Int31()) + messageSender := &fakeMessageSender{nil, fullClosedChan, resetChan, messagesSent} + var waitGroup sync.WaitGroup + messageNetwork := &fakeMessageNetwork{nil, nil, messageSender, &waitGroup} + allocator := allocator2.NewAllocator(1<<30, 1<<30) + + // we use only a retry count of 1 to avoid multiple send attempts for each message + messageQueue := New(ctx, peer, messageNetwork, allocator, 1, sendMessageTimeout) + messageQueue.Startup() + waitGroup.Add(1) + + // generate large blocks before proceeding + blks := testutil.GenerateBlocksOfSize(5, 1000000) + expectedTopic := "testTopic" + notifee, verifier := testutil.NewTestNotifee(expectedTopic, 5) + + messageQueue.AllocateAndBuildMessage(uint64(len(blks[0].RawData())), func(b *gsmsg.Builder) { + b.AddBlock(blks[0]) + b.AddLink(requestID1, cidlink.Link{Cid: blks[0].Cid()}, true) + }, []notifications.Notifee{notifee}) + waitGroup.Wait() + var message gsmsg.GraphSyncMessage + testutil.AssertReceive(ctx, t, messagesSent, &message, "message did not send") + + msgBlks := message.Blocks() + require.Len(t, msgBlks, 1, "number of blks in first message was not 1") + require.True(t, blks[0].Cid().Equals(msgBlks[0].Cid())) + + verifier.ExpectEvents(ctx, t, []notifications.Event{ + Event{Name: Queued}, + Event{Name: Sent}, + }) + verifier.ExpectClose(ctx, t) + fc1 := &fakeCloser{fms: messageSender} + fc2 := &fakeCloser{fms: messageSender} + // Send 3 very large blocks + messageQueue.AllocateAndBuildMessage(uint64(len(blks[1].RawData())), func(b *gsmsg.Builder) { + b.AddBlock(blks[1]) + b.AddResponseStream(requestID1, fc1) + b.AddLink(requestID1, cidlink.Link{Cid: blks[1].Cid()}, true) + }, []notifications.Notifee{}) + messageQueue.AllocateAndBuildMessage(uint64(len(blks[2].RawData())), func(b *gsmsg.Builder) { + b.AddBlock(blks[2]) + b.AddResponseStream(requestID1, fc1) + b.AddLink(requestID1, cidlink.Link{Cid: blks[2].Cid()}, true) + }, []notifications.Notifee{}) + messageQueue.AllocateAndBuildMessage(uint64(len(blks[3].RawData())), func(b *gsmsg.Builder) { + b.AddResponseStream(requestID2, fc2) + b.AddLink(requestID2, cidlink.Link{Cid: blks[3].Cid()}, true) + b.AddBlock(blks[3]) + }, []notifications.Notifee{}) + + messageSender.sendError = errors.New("something went wrong") + + // add one since the stream will get reopened + waitGroup.Add(1) + + testutil.AssertReceive(ctx, t, messagesSent, &message, "message did not send") + msgBlks = message.Blocks() + require.Len(t, msgBlks, 1, "number of blks in first message was not 1") + require.True(t, blks[1].Cid().Equals(msgBlks[0].Cid())) + + // should skip block2 as it's linked to errored request + testutil.AssertReceive(ctx, t, messagesSent, &message, "message did not send") + msgBlks = message.Blocks() + require.Len(t, msgBlks, 1, "number of blks in first message was not 1") + require.True(t, blks[3].Cid().Equals(msgBlks[0].Cid())) + + require.True(t, fc1.closed) + require.False(t, fc2.closed) +} + +const sendMessageTimeout = 10 * time.Minute +const messageSendRetries = 10 + +type fakeMessageNetwork struct { + connectError error + messageSenderError error + messageSender gsnet.MessageSender + wait *sync.WaitGroup +} + +func (fmn *fakeMessageNetwork) ConnectTo(context.Context, peer.ID) error { + return fmn.connectError +} + +func (fmn *fakeMessageNetwork) NewMessageSender(context.Context, peer.ID, gsnet.MessageSenderOpts) (gsnet.MessageSender, error) { + fmn.wait.Done() + if fmn.messageSenderError == nil { + return fmn.messageSender, nil + } + return nil, fmn.messageSenderError +} + +type fakeMessageSender struct { + sendError error + fullClosed chan<- struct{} + reset chan<- struct{} + messagesSent chan<- gsmsg.GraphSyncMessage +} + +func (fms *fakeMessageSender) SendMsg(ctx context.Context, msg gsmsg.GraphSyncMessage) error { + fms.messagesSent <- msg + return fms.sendError +} +func (fms *fakeMessageSender) Close() error { fms.fullClosed <- struct{}{}; return nil } +func (fms *fakeMessageSender) Reset() error { fms.reset <- struct{}{}; return nil } + +type fakeCloser struct { + fms *fakeMessageSender + closed bool +} + +func (fc *fakeCloser) Close() error { + fc.closed = true + // clear error so the next send goes through + fc.fms.sendError = nil + return nil +} From 7d7ee5b62833faeb923b35f5e702f5f62bdcead0 Mon Sep 17 00:00:00 2001 From: hannahhoward Date: Tue, 14 Dec 2021 22:27:44 -0800 Subject: [PATCH 03/12] refactor(responseassembler): use streams --- impl/graphsync.go | 1 - messagequeue/messagequeue.go | 18 -- responsemanager/client.go | 28 ++-- .../queryexecutor/queryexecutor.go | 46 +++-- .../queryexecutor/queryexecutor_test.go | 25 ++- responsemanager/querypreparer.go | 33 ++-- .../responseassembler/responseassembler.go | 73 ++++++-- .../responseassembler_test.go | 157 +++++++++++++----- responsemanager/responsemanager_test.go | 33 ++-- responsemanager/server.go | 30 ++-- 10 files changed, 276 insertions(+), 168 deletions(-) diff --git a/impl/graphsync.go b/impl/graphsync.go index e2d3b328..0f546b24 100644 --- a/impl/graphsync.go +++ b/impl/graphsync.go @@ -259,7 +259,6 @@ func New(parent context.Context, network gsnet.GraphSyncNetwork, responseManager, outgoingBlockHooks, requestUpdatedHooks, - responseAssembler, ) graphSync := &GraphSync{ network: network, diff --git a/messagequeue/messagequeue.go b/messagequeue/messagequeue.go index f7cfb4e4..0c9cd980 100644 --- a/messagequeue/messagequeue.go +++ b/messagequeue/messagequeue.go @@ -98,24 +98,6 @@ func (mq *MessageQueue) AllocateAndBuildMessage(size uint64, buildMessageFn func } } -type responseStream struct { - requestID graphsync.RequestID - closed bool - closedLk sync.RWMutex -} - -func (r *responseStream) close() { - r.closedLk.Lock() - r.closed = true - r.closedLk.Unlock() -} - -func (r *responseStream) isClosed() bool { - r.closedLk.RLock() - defer r.closedLk.RUnlock() - return r.closed -} - func (mq *MessageQueue) buildMessage(size uint64, buildMessageFn func(*gsmsg.Builder), notifees []notifications.Notifee) bool { mq.buildersLk.Lock() defer mq.buildersLk.Unlock() diff --git a/responsemanager/client.go b/responsemanager/client.go index 5b9ffe95..c3bf92a5 100644 --- a/responsemanager/client.go +++ b/responsemanager/client.go @@ -30,17 +30,18 @@ import ( var log = logging.Logger("graphsync") type inProgressResponseStatus struct { - ctx context.Context - span trace.Span - cancelFn func() - request gsmsg.GraphSyncRequest - loader ipld.BlockReadOpener - traverser ipldutil.Traverser - signals queryexecutor.ResponseSignals - updates []gsmsg.GraphSyncRequest - state graphsync.RequestState - subscriber *notifications.TopicDataSubscriber - startTime time.Time + ctx context.Context + span trace.Span + cancelFn func() + request gsmsg.GraphSyncRequest + loader ipld.BlockReadOpener + traverser ipldutil.Traverser + signals queryexecutor.ResponseSignals + updates []gsmsg.GraphSyncRequest + state graphsync.RequestState + subscriber *notifications.TopicDataSubscriber + startTime time.Time + responseStream responseassembler.ResponseStream } type responseKey struct { @@ -85,10 +86,7 @@ type NetworkErrorListeners interface { // ResponseAssembler is an interface that returns sender interfaces for peer responses. type ResponseAssembler interface { - DedupKey(p peer.ID, requestID graphsync.RequestID, key string) - IgnoreBlocks(p peer.ID, requestID graphsync.RequestID, links []ipld.Link) - SkipFirstBlocks(p peer.ID, requestID graphsync.RequestID, skipCount int64) - Transaction(p peer.ID, requestID graphsync.RequestID, transaction responseassembler.Transaction) error + NewStream(p peer.ID, requestID graphsync.RequestID) responseassembler.ResponseStream } type responseManagerMessage interface { diff --git a/responsemanager/queryexecutor/queryexecutor.go b/responsemanager/queryexecutor/queryexecutor.go index e592bf8f..c1be1ff8 100644 --- a/responsemanager/queryexecutor/queryexecutor.go +++ b/responsemanager/queryexecutor/queryexecutor.go @@ -38,14 +38,15 @@ const ErrFirstBlockLoad = errorString("Unable to load first block") // ResponseTask returns all information needed to execute a given response type ResponseTask struct { - Empty bool - Subscriber *notifications.TopicDataSubscriber - Ctx context.Context - Span trace.Span - Request gsmsg.GraphSyncRequest - Loader ipld.BlockReadOpener - Traverser ipldutil.Traverser - Signals ResponseSignals + Empty bool + Subscriber *notifications.TopicDataSubscriber + Ctx context.Context + Span trace.Span + Request gsmsg.GraphSyncRequest + Loader ipld.BlockReadOpener + Traverser ipldutil.Traverser + Signals ResponseSignals + ResponseStream ResponseStream } // ResponseSignals are message channels to communicate between the manager and the QueryExecutor @@ -57,11 +58,10 @@ type ResponseSignals struct { // QueryExecutor is responsible for performing individual requests by executing their traversals type QueryExecutor struct { - ctx context.Context - manager Manager - blockHooks BlockHooks - updateHooks UpdateHooks - responseAssembler ResponseAssembler + ctx context.Context + manager Manager + blockHooks BlockHooks + updateHooks UpdateHooks } // New creates a new QueryExecutor @@ -69,14 +69,12 @@ func New(ctx context.Context, manager Manager, blockHooks BlockHooks, updateHooks UpdateHooks, - responseAssembler ResponseAssembler, ) *QueryExecutor { qm := &QueryExecutor{ - blockHooks: blockHooks, - updateHooks: updateHooks, - responseAssembler: responseAssembler, - manager: manager, - ctx: ctx, + blockHooks: blockHooks, + updateHooks: updateHooks, + manager: manager, + ctx: ctx, } return qm } @@ -124,7 +122,7 @@ func (qe *QueryExecutor) executeQuery( err := qe.runTraversal(p, rt) // Close out the response, either temporarily (pause) or permanently (cancel, fail, complete) - return qe.responseAssembler.Transaction(p, rt.Request.ID(), func(rb responseassembler.ResponseBuilder) error { + return rt.ResponseStream.Transaction(func(rb responseassembler.ResponseBuilder) error { var code graphsync.ResponseStatusCode if err != nil { _, isPaused := err.(hooks.ErrPaused) @@ -245,7 +243,7 @@ func (qe *QueryExecutor) nextBlock(taskData ResponseTask) (ipld.Link, []byte, er func (qe *QueryExecutor) sendResponse(p peer.ID, taskData ResponseTask, link ipld.Link, data []byte) error { // Execute a transaction for this block, including any other queued operations - return qe.responseAssembler.Transaction(p, taskData.Request.ID(), func(rb responseassembler.ResponseBuilder) error { + return taskData.ResponseStream.Transaction(func(rb responseassembler.ResponseBuilder) error { // Ensure that any updates that have occurred till now are integrated into the response err := qe.checkForUpdates(p, taskData, rb) // On any error other than a pause, we bail, if it's a pause then we continue processing _this_ block @@ -287,7 +285,7 @@ type UpdateHooks interface { ProcessUpdateHooks(p peer.ID, request graphsync.RequestData, update graphsync.RequestData) hooks.UpdateResult } -// ResponseAssembler is an interface that returns sender interfaces for peer responses. -type ResponseAssembler interface { - Transaction(p peer.ID, requestID graphsync.RequestID, transaction responseassembler.Transaction) error +// ResponseStream is an interface that returns sender interfaces for peer responses. +type ResponseStream interface { + Transaction(transaction responseassembler.Transaction) error } diff --git a/responsemanager/queryexecutor/queryexecutor_test.go b/responsemanager/queryexecutor/queryexecutor_test.go index 3e3f2922..b7603a99 100644 --- a/responsemanager/queryexecutor/queryexecutor_test.go +++ b/responsemanager/queryexecutor/queryexecutor_test.go @@ -60,7 +60,7 @@ func TestSmallGraphTask(t *testing.T) { transactionExpect := func(t *testing.T, td *testData, errorAt []int, errorStr string) { var transactionCalls int - td.responseAssembler.transactionCb = func(e error) { + td.responseStream.transactionCb = func(e error) { var erroredAt bool for _, i := range errorAt { if transactionCalls == i { @@ -272,7 +272,7 @@ type testData struct { blockStore map[ipld.Link][]byte persistence ipld.LinkSystem manager *fauxManager - responseAssembler *fauxResponseAssembler + responseStream *fauxResponseStream responseBuilder *fauxResponseBuilder blockHooks *hooks.OutgoingBlockHooks updateHooks *hooks.RequestUpdatedHooks @@ -302,7 +302,6 @@ func newTestData(t *testing.T, blockCount int, expectedTraverse int) (*testData, td.persistence = testutil.NewTestStore(td.blockStore) td.task = &peertask.Task{} td.manager = &fauxManager{ctx: ctx, t: t, expectedStartTask: td.task} - td.responseAssembler = &fauxResponseAssembler{} td.blockHooks = hooks.NewBlockHooks() td.updateHooks = hooks.NewUpdateHooks() td.requestID = graphsync.RequestID(rand.Int31()) @@ -353,7 +352,7 @@ func newTestData(t *testing.T, blockCount int, expectedTraverse int) (*testData, }, } - td.responseAssembler = &fauxResponseAssembler{ + td.responseStream = &fauxResponseStream{ t: t, responseBuilder: td.responseBuilder, } @@ -375,13 +374,14 @@ func newTestData(t *testing.T, blockCount int, expectedTraverse int) (*testData, } td.manager.responseTask = ResponseTask{ - Request: td.requests[0], - Loader: loader, - Traverser: expectedTraverser, - Signals: *td.signals, - Subscriber: td.subscriber, + Request: td.requests[0], + Loader: loader, + Traverser: expectedTraverser, + Signals: *td.signals, + Subscriber: td.subscriber, + ResponseStream: td.responseStream, } - td.responseAssembler.responseBuilder.pauseCb = func() { + td.responseStream.responseBuilder.pauseCb = func() { td.pauseCalls++ } @@ -390,7 +390,6 @@ func newTestData(t *testing.T, blockCount int, expectedTraverse int) (*testData, td.manager, td.blockHooks, td.updateHooks, - td.responseAssembler, ) return td, qe } @@ -418,13 +417,13 @@ func (fm *fauxManager) GetUpdates(p peer.ID, requestID graphsync.RequestID, upda func (fm *fauxManager) FinishTask(task *peertask.Task, err error) { } -type fauxResponseAssembler struct { +type fauxResponseStream struct { t *testing.T responseBuilder *fauxResponseBuilder transactionCb func(error) } -func (fra *fauxResponseAssembler) Transaction(p peer.ID, requestID graphsync.RequestID, transaction responseassembler.Transaction) error { +func (fra *fauxResponseStream) Transaction(transaction responseassembler.Transaction) error { var err error if fra.responseBuilder != nil { err = transaction(fra.responseBuilder) diff --git a/responsemanager/querypreparer.go b/responsemanager/querypreparer.go index e74f2681..7afb7a24 100644 --- a/responsemanager/querypreparer.go +++ b/responsemanager/querypreparer.go @@ -30,9 +30,8 @@ func (e errorString) Error() string { const errInvalidRequest = errorString("request not valid") type queryPreparer struct { - requestHooks RequestHooks - responseAssembler ResponseAssembler - linkSystem ipld.LinkSystem + requestHooks RequestHooks + linkSystem ipld.LinkSystem // maximum number of links to traverse per request. A value of zero = infinity, or no limit= maxLinksPerRequest uint64 } @@ -41,14 +40,14 @@ func (qe *queryPreparer) prepareQuery( ctx context.Context, p peer.ID, request gsmsg.GraphSyncRequest, + responseStream responseassembler.ResponseStream, signals queryexecutor.ResponseSignals, sub *notifications.TopicDataSubscriber) (ipld.BlockReadOpener, ipldutil.Traverser, bool, error) { - result := qe.requestHooks.ProcessRequestHooks(p, request) var isPaused bool failNotifee := notifications.Notifee{Data: graphsync.RequestFailedUnknown, Subscriber: sub} rejectNotifee := notifications.Notifee{Data: graphsync.RequestRejected, Subscriber: sub} - err := qe.responseAssembler.Transaction(p, request.ID(), func(rb responseassembler.ResponseBuilder) error { + err := responseStream.Transaction(func(rb responseassembler.ResponseBuilder) error { for _, extension := range result.Extensions { rb.SendExtensionData(extension) } @@ -69,13 +68,13 @@ func (qe *queryPreparer) prepareQuery( if err != nil { return nil, nil, false, err } - if err := qe.processDedupByKey(request, p, failNotifee); err != nil { + if err := processDedupByKey(request, responseStream, failNotifee); err != nil { return nil, nil, false, err } - if err := qe.processDoNoSendCids(request, p, failNotifee); err != nil { + if err := processDoNoSendCids(request, responseStream, failNotifee); err != nil { return nil, nil, false, err } - if err := qe.processDoNotSendFirstBlocks(request, p, failNotifee); err != nil { + if err := processDoNotSendFirstBlocks(request, responseStream, failNotifee); err != nil { return nil, nil, false, err } rootLink := cidlink.Link{Cid: request.Root()} @@ -101,32 +100,32 @@ func (qe *queryPreparer) prepareQuery( return linkSystem.StorageReadOpener, traverser, isPaused, nil } -func (qe *queryPreparer) processDedupByKey(request gsmsg.GraphSyncRequest, p peer.ID, failNotifee notifications.Notifee) error { +func processDedupByKey(request gsmsg.GraphSyncRequest, responseStream responseassembler.ResponseStream, failNotifee notifications.Notifee) error { dedupData, has := request.Extension(graphsync.ExtensionDeDupByKey) if !has { return nil } key, err := dedupkey.DecodeDedupKey(dedupData) if err != nil { - _ = qe.responseAssembler.Transaction(p, request.ID(), func(rb responseassembler.ResponseBuilder) error { + _ = responseStream.Transaction(func(rb responseassembler.ResponseBuilder) error { rb.FinishWithError(graphsync.RequestFailedUnknown) rb.AddNotifee(failNotifee) return nil }) return err } - qe.responseAssembler.DedupKey(p, request.ID(), key) + responseStream.DedupKey(key) return nil } -func (qe *queryPreparer) processDoNoSendCids(request gsmsg.GraphSyncRequest, p peer.ID, failNotifee notifications.Notifee) error { +func processDoNoSendCids(request gsmsg.GraphSyncRequest, responseStream responseassembler.ResponseStream, failNotifee notifications.Notifee) error { doNotSendCidsData, has := request.Extension(graphsync.ExtensionDoNotSendCIDs) if !has { return nil } cidSet, err := cidset.DecodeCidSet(doNotSendCidsData) if err != nil { - _ = qe.responseAssembler.Transaction(p, request.ID(), func(rb responseassembler.ResponseBuilder) error { + _ = responseStream.Transaction(func(rb responseassembler.ResponseBuilder) error { rb.FinishWithError(graphsync.RequestFailedUnknown) rb.AddNotifee(failNotifee) return nil @@ -141,24 +140,24 @@ func (qe *queryPreparer) processDoNoSendCids(request gsmsg.GraphSyncRequest, p p if err != nil { return err } - qe.responseAssembler.IgnoreBlocks(p, request.ID(), links) + responseStream.IgnoreBlocks(links) return nil } -func (qe *queryPreparer) processDoNotSendFirstBlocks(request gsmsg.GraphSyncRequest, p peer.ID, failNotifee notifications.Notifee) error { +func processDoNotSendFirstBlocks(request gsmsg.GraphSyncRequest, responseStream responseassembler.ResponseStream, failNotifee notifications.Notifee) error { doNotSendFirstBlocksData, has := request.Extension(graphsync.ExtensionsDoNotSendFirstBlocks) if !has { return nil } skipCount, err := donotsendfirstblocks.DecodeDoNotSendFirstBlocks(doNotSendFirstBlocksData) if err != nil { - _ = qe.responseAssembler.Transaction(p, request.ID(), func(rb responseassembler.ResponseBuilder) error { + _ = responseStream.Transaction(func(rb responseassembler.ResponseBuilder) error { rb.FinishWithError(graphsync.RequestFailedUnknown) rb.AddNotifee(failNotifee) return nil }) return err } - qe.responseAssembler.SkipFirstBlocks(p, request.ID(), skipCount) + responseStream.SkipFirstBlocks(skipCount) return nil } diff --git a/responsemanager/responseassembler/responseassembler.go b/responsemanager/responseassembler/responseassembler.go index 3c557e7e..4b02e0c8 100644 --- a/responsemanager/responseassembler/responseassembler.go +++ b/responsemanager/responseassembler/responseassembler.go @@ -9,6 +9,7 @@ package responseassembler import ( "context" + "sync" "github.com/ipld/go-ipld-prime" "github.com/libp2p/go-libp2p-core/peer" @@ -61,7 +62,6 @@ type PeerMessageHandler interface { type ResponseAssembler struct { *peermanager.PeerManager peerHandler PeerMessageHandler - ctx context.Context } // New generates a new ResponseAssembler for sending responses @@ -70,46 +70,89 @@ func New(ctx context.Context, peerHandler PeerMessageHandler) *ResponseAssembler PeerManager: peermanager.New(ctx, func(ctx context.Context, p peer.ID) peermanager.PeerHandler { return newTracker() }), - ctx: ctx, peerHandler: peerHandler, } } +func (ra *ResponseAssembler) NewStream(p peer.ID, requestID graphsync.RequestID) ResponseStream { + return &responseStream{ + requestID: requestID, + p: p, + messageSenders: ra.peerHandler, + linkTrackers: ra.PeerManager, + } +} + +type responseStream struct { + requestID graphsync.RequestID + p peer.ID + closed bool + closedLk sync.RWMutex + messageSenders PeerMessageHandler + linkTrackers *peermanager.PeerManager +} + +func (r *responseStream) Close() error { + r.closedLk.Lock() + r.closed = true + r.closedLk.Unlock() + return nil +} + +func (r *responseStream) isClosed() bool { + r.closedLk.RLock() + defer r.closedLk.RUnlock() + return r.closed +} + +type ResponseStream interface { + Transaction(transaction Transaction) error + DedupKey(key string) + IgnoreBlocks(links []ipld.Link) + SkipFirstBlocks(skipFirstBlocks int64) +} + // DedupKey indicates that outgoing blocks should be deduplicated in a seperate bucket (only with requests that share // supplied key string) -func (ra *ResponseAssembler) DedupKey(p peer.ID, requestID graphsync.RequestID, key string) { - ra.GetProcess(p).(*peerLinkTracker).DedupKey(requestID, key) +func (rs *responseStream) DedupKey(key string) { + rs.linkTrackers.GetProcess(rs.p).(*peerLinkTracker).DedupKey(rs.requestID, key) } // IgnoreBlocks indicates that a list of keys should be ignored when sending blocks -func (ra *ResponseAssembler) IgnoreBlocks(p peer.ID, requestID graphsync.RequestID, links []ipld.Link) { - ra.GetProcess(p).(*peerLinkTracker).IgnoreBlocks(requestID, links) +func (rs *responseStream) IgnoreBlocks(links []ipld.Link) { + rs.linkTrackers.GetProcess(rs.p).(*peerLinkTracker).IgnoreBlocks(rs.requestID, links) } // SkipFirstBlocks tells the assembler for the given request to not send the first N blocks -func (ra *ResponseAssembler) SkipFirstBlocks(p peer.ID, requestID graphsync.RequestID, skipFirstBlocks int64) { - ra.GetProcess(p).(*peerLinkTracker).SkipFirstBlocks(requestID, skipFirstBlocks) +func (rs *responseStream) SkipFirstBlocks(skipFirstBlocks int64) { + rs.linkTrackers.GetProcess(rs.p).(*peerLinkTracker).SkipFirstBlocks(rs.requestID, skipFirstBlocks) } -// Transaction builds a response, and queues it for sending in the next outgoing message -func (ra *ResponseAssembler) Transaction(p peer.ID, requestID graphsync.RequestID, transaction Transaction) error { +func (rs *responseStream) Transaction(transaction Transaction) error { rb := &responseBuilder{ - requestID: requestID, - linkTracker: ra.GetProcess(p).(*peerLinkTracker), + requestID: rs.requestID, + linkTracker: rs.linkTrackers.GetProcess(rs.p).(*peerLinkTracker), } err := transaction(rb) - ra.execute(p, rb.operations, rb.notifees) + rs.execute(rb.operations, rb.notifees) return err } -func (ra *ResponseAssembler) execute(p peer.ID, operations []responseOperation, notifees []notifications.Notifee) { +func (rs *responseStream) execute(operations []responseOperation, notifees []notifications.Notifee) { size := uint64(0) for _, op := range operations { size += op.size() } - ra.peerHandler.AllocateAndBuildMessage(p, size, func(builder *gsmsg.Builder) { + if rs.isClosed() { + return + } + rs.messageSenders.AllocateAndBuildMessage(rs.p, size, func(builder *gsmsg.Builder) { + if rs.isClosed() { + return + } for _, op := range operations { op.build(builder) } + builder.AddResponseStream(rs.requestID, rs) }, notifees) } diff --git a/responsemanager/responseassembler/responseassembler_test.go b/responsemanager/responseassembler/responseassembler_test.go index b13cbd15..bc88e15a 100644 --- a/responsemanager/responseassembler/responseassembler_test.go +++ b/responsemanager/responseassembler/responseassembler_test.go @@ -3,6 +3,7 @@ package responseassembler import ( "context" "fmt" + "io" "math/rand" "testing" "time" @@ -41,8 +42,12 @@ func TestResponseAssemblerSendsResponses(t *testing.T) { var bd1, bd2 graphsync.BlockData + stream1 := responseAssembler.NewStream(p, requestID1) + stream2 := responseAssembler.NewStream(p, requestID2) + stream3 := responseAssembler.NewStream(p, requestID3) + // send block 0 for request 1 - require.NoError(t, responseAssembler.Transaction(p, requestID1, func(b ResponseBuilder) error { + require.NoError(t, stream1.Transaction(func(b ResponseBuilder) error { b.AddNotifee(sendResponseNotifee1) bd1 = b.SendResponse(links[0], blks[0].RawData()) return nil @@ -51,9 +56,9 @@ func TestResponseAssemblerSendsResponses(t *testing.T) { fph.AssertBlocks(blks[0]) fph.AssertResponses(expectedResponses{requestID1: graphsync.PartialResponse}) fph.AssertNotifees(sendResponseNotifee1) - + fph.AssertResponseStream(requestID1, stream1) // send block 0 for request 2 (duplicate block should not be sent) - require.NoError(t, responseAssembler.Transaction(p, requestID2, func(b ResponseBuilder) error { + require.NoError(t, stream2.Transaction(func(b ResponseBuilder) error { b.AddNotifee(sendResponseNotifee2) bd1 = b.SendResponse(links[0], blks[0].RawData()) return nil @@ -61,9 +66,10 @@ func TestResponseAssemblerSendsResponses(t *testing.T) { assertSentNotOnWire(t, bd1, blks[0]) fph.AssertResponses(expectedResponses{requestID2: graphsync.PartialResponse}) fph.AssertNotifees(sendResponseNotifee2) + fph.AssertResponseStream(requestID2, stream2) // send more to request 1 and finish request - require.NoError(t, responseAssembler.Transaction(p, requestID1, func(b ResponseBuilder) error { + require.NoError(t, stream1.Transaction(func(b ResponseBuilder) error { // send block 1 bd1 = b.SendResponse(links[1], blks[1].RawData()) // block 2 is not found. Assert not sent @@ -77,9 +83,10 @@ func TestResponseAssemblerSendsResponses(t *testing.T) { fph.AssertResponses(expectedResponses{ requestID1: graphsync.RequestCompletedPartial, }) + fph.AssertResponseStream(requestID1, stream1) // send more to request 2 - require.NoError(t, responseAssembler.Transaction(p, requestID2, func(b ResponseBuilder) error { + require.NoError(t, stream2.Transaction(func(b ResponseBuilder) error { bd1 = b.SendResponse(links[3], blks[3].RawData()) b.FinishRequest() return nil @@ -88,9 +95,10 @@ func TestResponseAssemblerSendsResponses(t *testing.T) { fph.AssertResponses(expectedResponses{ requestID2: graphsync.RequestCompletedFull, }) + fph.AssertResponseStream(requestID2, stream2) // send to request 3 - require.NoError(t, responseAssembler.Transaction(p, requestID3, func(b ResponseBuilder) error { + require.NoError(t, stream3.Transaction(func(b ResponseBuilder) error { bd1 = b.SendResponse(links[4], blks[4].RawData()) return nil })) @@ -98,9 +106,10 @@ func TestResponseAssemblerSendsResponses(t *testing.T) { fph.AssertResponses(expectedResponses{ requestID3: graphsync.PartialResponse, }) + fph.AssertResponseStream(requestID3, stream3) // send 2 more to request 3 - require.NoError(t, responseAssembler.Transaction(p, requestID3, func(b ResponseBuilder) error { + require.NoError(t, stream3.Transaction(func(b ResponseBuilder) error { b.AddNotifee(sendResponseNotifee3) bd1 = b.SendResponse(links[0], blks[0].RawData()) bd1 = b.SendResponse(links[4], blks[4].RawData()) @@ -109,8 +118,43 @@ func TestResponseAssemblerSendsResponses(t *testing.T) { fph.AssertBlocks(blks[0]) fph.AssertResponses(expectedResponses{requestID3: graphsync.PartialResponse}) + fph.AssertResponseStream(requestID3, stream3) } +func TestResponseAssemblerCloseStream(t *testing.T) { + ctx := context.Background() + ctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + p := testutil.GeneratePeers(1)[0] + requestID1 := graphsync.RequestID(rand.Int31()) + blks := testutil.GenerateBlocksOfSize(5, 100) + links := make([]ipld.Link, 0, len(blks)) + for _, block := range blks { + links = append(links, cidlink.Link{Cid: block.Cid()}) + } + fph := newFakePeerHandler(ctx, t) + responseAssembler := New(ctx, fph) + + stream1 := responseAssembler.NewStream(p, requestID1) + require.NoError(t, stream1.Transaction(func(b ResponseBuilder) error { + b.SendResponse(links[0], blks[0].RawData()) + return nil + })) + fph.AssertBlocks(blks[0]) + fph.AssertResponses(expectedResponses{requestID1: graphsync.PartialResponse}) + fph.AssertResponseStream(requestID1, stream1) + + // close the response stream + fph.CloseResponseStream(requestID1) + fph.Clear() + + require.NoError(t, stream1.Transaction(func(b ResponseBuilder) error { + b.SendResponse(links[1], blks[1].RawData()) + return nil + })) + fph.RefuteBlocks() + fph.RefuteResponses() +} func TestResponseAssemblerSendsExtensionData(t *testing.T) { ctx := context.Background() ctx, cancel := context.WithTimeout(ctx, 10*time.Second) @@ -125,7 +169,8 @@ func TestResponseAssemblerSendsExtensionData(t *testing.T) { fph := newFakePeerHandler(ctx, t) responseAssembler := New(ctx, fph) - require.NoError(t, responseAssembler.Transaction(p, requestID1, func(b ResponseBuilder) error { + stream1 := responseAssembler.NewStream(p, requestID1) + require.NoError(t, stream1.Transaction(func(b ResponseBuilder) error { b.SendResponse(links[0], blks[0].RawData()) return nil })) @@ -145,7 +190,7 @@ func TestResponseAssemblerSendsExtensionData(t *testing.T) { Name: extensionName2, Data: extensionData2, } - require.NoError(t, responseAssembler.Transaction(p, requestID1, func(b ResponseBuilder) error { + require.NoError(t, stream1.Transaction(func(b ResponseBuilder) error { b.SendResponse(links[1], blks[1].RawData()) b.SendExtensionData(extension1) b.SendExtensionData(extension2) @@ -170,7 +215,8 @@ func TestResponseAssemblerSendsResponsesInTransaction(t *testing.T) { fph := newFakePeerHandler(ctx, t) responseAssembler := New(ctx, fph) notifee, _ := testutil.NewTestNotifee("transaction", 10) - err := responseAssembler.Transaction(p, requestID1, func(b ResponseBuilder) error { + stream1 := responseAssembler.NewStream(p, requestID1) + err := stream1.Transaction(func(b ResponseBuilder) error { bd := b.SendResponse(links[0], blks[0].RawData()) assertSentOnWire(t, bd, blks[0]) @@ -207,11 +253,14 @@ func TestResponseAssemblerIgnoreBlocks(t *testing.T) { } fph := newFakePeerHandler(ctx, t) responseAssembler := New(ctx, fph) + stream1 := responseAssembler.NewStream(p, requestID1) + stream2 := responseAssembler.NewStream(p, requestID2) - responseAssembler.IgnoreBlocks(p, requestID1, links) + stream1.IgnoreBlocks(links) var bd1, bd2, bd3 graphsync.BlockData - err := responseAssembler.Transaction(p, requestID1, func(b ResponseBuilder) error { + + err := stream1.Transaction(func(b ResponseBuilder) error { bd1 = b.SendResponse(links[0], blks[0].RawData()) return nil }) @@ -221,7 +270,7 @@ func TestResponseAssemblerIgnoreBlocks(t *testing.T) { fph.RefuteBlocks() fph.AssertResponses(expectedResponses{requestID1: graphsync.PartialResponse}) - err = responseAssembler.Transaction(p, requestID2, func(b ResponseBuilder) error { + err = stream2.Transaction(func(b ResponseBuilder) error { bd1 = b.SendResponse(links[0], blks[0].RawData()) return nil }) @@ -230,7 +279,7 @@ func TestResponseAssemblerIgnoreBlocks(t *testing.T) { requestID2: graphsync.PartialResponse, }) - err = responseAssembler.Transaction(p, requestID1, func(b ResponseBuilder) error { + err = stream1.Transaction(func(b ResponseBuilder) error { bd2 = b.SendResponse(links[1], blks[1].RawData()) bd3 = b.SendResponse(links[2], blks[2].RawData()) b.FinishRequest() @@ -247,7 +296,7 @@ func TestResponseAssemblerIgnoreBlocks(t *testing.T) { requestID1: graphsync.RequestCompletedFull, }) - err = responseAssembler.Transaction(p, requestID2, func(b ResponseBuilder) error { + err = stream2.Transaction(func(b ResponseBuilder) error { b.SendResponse(links[3], blks[3].RawData()) b.FinishRequest() return nil @@ -274,10 +323,14 @@ func TestResponseAssemblerSkipFirstBlocks(t *testing.T) { fph := newFakePeerHandler(ctx, t) responseAssembler := New(ctx, fph) - responseAssembler.SkipFirstBlocks(p, requestID1, 3) + stream1 := responseAssembler.NewStream(p, requestID1) + stream2 := responseAssembler.NewStream(p, requestID2) + + stream1.SkipFirstBlocks(3) var bd1, bd2, bd3, bd4, bd5 graphsync.BlockData - err := responseAssembler.Transaction(p, requestID1, func(b ResponseBuilder) error { + + err := stream1.Transaction(func(b ResponseBuilder) error { bd1 = b.SendResponse(links[0], blks[0].RawData()) return nil }) @@ -287,7 +340,7 @@ func TestResponseAssemblerSkipFirstBlocks(t *testing.T) { fph.RefuteBlocks() fph.AssertResponses(expectedResponses{requestID1: graphsync.PartialResponse}) - err = responseAssembler.Transaction(p, requestID2, func(b ResponseBuilder) error { + err = stream2.Transaction(func(b ResponseBuilder) error { bd1 = b.SendResponse(links[0], blks[0].RawData()) return nil }) @@ -296,7 +349,7 @@ func TestResponseAssemblerSkipFirstBlocks(t *testing.T) { requestID2: graphsync.PartialResponse, }) - err = responseAssembler.Transaction(p, requestID1, func(b ResponseBuilder) error { + err = stream1.Transaction(func(b ResponseBuilder) error { bd2 = b.SendResponse(links[1], blks[1].RawData()) bd3 = b.SendResponse(links[2], blks[2].RawData()) return nil @@ -311,7 +364,7 @@ func TestResponseAssemblerSkipFirstBlocks(t *testing.T) { fph.AssertResponses(expectedResponses{ requestID1: graphsync.PartialResponse, }) - err = responseAssembler.Transaction(p, requestID1, func(b ResponseBuilder) error { + err = stream1.Transaction(func(b ResponseBuilder) error { bd4 = b.SendResponse(links[3], blks[3].RawData()) bd5 = b.SendResponse(links[4], blks[4].RawData()) b.FinishRequest() @@ -325,7 +378,7 @@ func TestResponseAssemblerSkipFirstBlocks(t *testing.T) { fph.AssertBlocks(blks[3], blks[4]) fph.AssertResponses(expectedResponses{requestID1: graphsync.RequestCompletedFull}) - err = responseAssembler.Transaction(p, requestID2, func(b ResponseBuilder) error { + err = stream2.Transaction(func(b ResponseBuilder) error { b.SendResponse(links[3], blks[3].RawData()) b.FinishRequest() return nil @@ -352,12 +405,16 @@ func TestResponseAssemblerDupKeys(t *testing.T) { } fph := newFakePeerHandler(ctx, t) responseAssembler := New(ctx, fph) + stream1 := responseAssembler.NewStream(p, requestID1) + stream2 := responseAssembler.NewStream(p, requestID2) + stream3 := responseAssembler.NewStream(p, requestID3) - responseAssembler.DedupKey(p, requestID1, "applesauce") - responseAssembler.DedupKey(p, requestID3, "applesauce") + stream1.DedupKey("applesauce") + stream3.DedupKey("applesauce") var bd1, bd2 graphsync.BlockData - err := responseAssembler.Transaction(p, requestID1, func(b ResponseBuilder) error { + + err := stream1.Transaction(func(b ResponseBuilder) error { bd1 = b.SendResponse(links[0], blks[0].RawData()) return nil }) @@ -367,14 +424,14 @@ func TestResponseAssemblerDupKeys(t *testing.T) { fph.AssertBlocks(blks[0]) fph.AssertResponses(expectedResponses{requestID1: graphsync.PartialResponse}) - err = responseAssembler.Transaction(p, requestID2, func(b ResponseBuilder) error { + err = stream2.Transaction(func(b ResponseBuilder) error { bd1 = b.SendResponse(links[0], blks[0].RawData()) return nil }) require.NoError(t, err) assertSentOnWire(t, bd1, blks[0]) - err = responseAssembler.Transaction(p, requestID1, func(b ResponseBuilder) error { + err = stream1.Transaction(func(b ResponseBuilder) error { bd1 = b.SendResponse(links[1], blks[1].RawData()) bd2 = b.SendResponse(links[2], nil) return nil @@ -386,7 +443,7 @@ func TestResponseAssemblerDupKeys(t *testing.T) { fph.AssertBlocks(blks[1]) fph.AssertResponses(expectedResponses{requestID1: graphsync.PartialResponse}) - err = responseAssembler.Transaction(p, requestID2, func(b ResponseBuilder) error { + err = stream2.Transaction(func(b ResponseBuilder) error { b.SendResponse(links[3], blks[3].RawData()) b.FinishRequest() return nil @@ -395,7 +452,7 @@ func TestResponseAssemblerDupKeys(t *testing.T) { fph.AssertBlocks(blks[3]) fph.AssertResponses(expectedResponses{requestID2: graphsync.RequestCompletedFull}) - err = responseAssembler.Transaction(p, requestID3, func(b ResponseBuilder) error { + err = stream3.Transaction(func(b ResponseBuilder) error { b.SendResponse(links[4], blks[4].RawData()) return nil }) @@ -403,7 +460,7 @@ func TestResponseAssemblerDupKeys(t *testing.T) { fph.AssertBlocks(blks[4]) fph.AssertResponses(expectedResponses{requestID3: graphsync.PartialResponse}) - err = responseAssembler.Transaction(p, requestID3, func(b ResponseBuilder) error { + err = stream3.Transaction(func(b ResponseBuilder) error { b.SendResponse(links[0], blks[0].RawData()) b.SendResponse(links[4], blks[4].RawData()) return nil @@ -445,19 +502,21 @@ func assertNotSent(t *testing.T, bd graphsync.BlockData, blk blocks.Block) { } type fakePeerHandler struct { - ctx context.Context - t *testing.T - lastBlocks []blocks.Block - lastResponses []gsmsg.GraphSyncResponse - lastNotifiees []notifications.Notifee - sent chan struct{} + ctx context.Context + t *testing.T + lastResponseStreams map[graphsync.RequestID]io.Closer + lastBlocks []blocks.Block + lastResponses []gsmsg.GraphSyncResponse + lastNotifiees []notifications.Notifee + sent chan struct{} } func newFakePeerHandler(ctx context.Context, t *testing.T) *fakePeerHandler { t.Helper() return &fakePeerHandler{ - ctx: ctx, - t: t, + lastResponseStreams: map[graphsync.RequestID]io.Closer{}, + ctx: ctx, + t: t, } } @@ -477,6 +536,18 @@ func (fph *fakePeerHandler) RefuteBlocks() { require.Empty(fph.t, fph.lastBlocks) } +func (fph *fakePeerHandler) AssertResponseStream(requestID graphsync.RequestID, expected ResponseStream) { + actual, ok := fph.lastResponseStreams[requestID] + require.True(fph.t, ok) + require.Equal(fph.t, expected, actual) +} + +func (fph *fakePeerHandler) CloseResponseStream(requestID graphsync.RequestID) { + actual, ok := fph.lastResponseStreams[requestID] + require.True(fph.t, ok) + actual.Close() +} + type expectedResponses map[graphsync.RequestID]graphsync.ResponseStatusCode func (fph *fakePeerHandler) AssertResponses(responses expectedResponses) { @@ -518,11 +589,19 @@ func (fph *fakePeerHandler) AllocateAndBuildMessage(p peer.ID, blkSize uint64, b msg, err := builder.Build() require.NoError(fph.t, err) - fph.sendResponse(p, msg.Responses(), msg.Blocks(), notifees...) + fph.sendResponse(p, msg.Responses(), msg.Blocks(), builder.ResponseStreams(), notifees...) } -func (fph *fakePeerHandler) sendResponse(p peer.ID, responses []gsmsg.GraphSyncResponse, blks []blocks.Block, notifees ...notifications.Notifee) { +func (fph *fakePeerHandler) sendResponse(p peer.ID, responses []gsmsg.GraphSyncResponse, blks []blocks.Block, responseStreams map[graphsync.RequestID]io.Closer, notifees ...notifications.Notifee) { fph.lastResponses = responses fph.lastBlocks = blks fph.lastNotifiees = notifees + fph.lastResponseStreams = responseStreams +} + +func (fph *fakePeerHandler) Clear() { + fph.lastResponses = nil + fph.lastNotifiees = nil + fph.lastResponseStreams = map[graphsync.RequestID]io.Closer{} + fph.lastBlocks = nil } diff --git a/responsemanager/responsemanager_test.go b/responsemanager/responsemanager_test.go index 2cc6f5f0..0af5c319 100644 --- a/responsemanager/responsemanager_test.go +++ b/responsemanager/responsemanager_test.go @@ -855,23 +855,32 @@ type fakeResponseAssembler struct { missingBlock bool } -func (fra *fakeResponseAssembler) Transaction(p peer.ID, requestID graphsync.RequestID, transaction responseassembler.Transaction) error { - fra.transactionLk.Lock() - defer fra.transactionLk.Unlock() - frb := &fakeResponseBuilder{requestID, fra, - fra.notifeePublisher} +func (fra *fakeResponseAssembler) NewStream(p peer.ID, requestID graphsync.RequestID) responseassembler.ResponseStream { + return &fakeResponseStream{fra, requestID} +} + +type fakeResponseStream struct { + fra *fakeResponseAssembler + requestID graphsync.RequestID +} + +func (frs *fakeResponseStream) Transaction(transaction responseassembler.Transaction) error { + frs.fra.transactionLk.Lock() + defer frs.fra.transactionLk.Unlock() + frb := &fakeResponseBuilder{frs.requestID, frs.fra, + frs.fra.notifeePublisher} return transaction(frb) } -func (fra *fakeResponseAssembler) IgnoreBlocks(p peer.ID, requestID graphsync.RequestID, links []ipld.Link) { - fra.ignoredLinks <- links +func (frs *fakeResponseStream) IgnoreBlocks(links []ipld.Link) { + frs.fra.ignoredLinks <- links } -func (fra *fakeResponseAssembler) SkipFirstBlocks(p peer.ID, requestID graphsync.RequestID, skipCount int64) { - fra.skippedFirstBlocks <- skipCount +func (frs *fakeResponseStream) SkipFirstBlocks(skipCount int64) { + frs.fra.skippedFirstBlocks <- skipCount } -func (fra *fakeResponseAssembler) DedupKey(p peer.ID, requestID graphsync.RequestID, key string) { - fra.dedupKeys <- key +func (frs *fakeResponseStream) DedupKey(key string) { + frs.fra.dedupKeys <- key } type sentResponse struct { @@ -1159,7 +1168,7 @@ func (td *testData) alternateLoaderResponseManager() *ResponseManager { } func (td *testData) newQueryExecutor(manager queryexecutor.Manager) *queryexecutor.QueryExecutor { - return queryexecutor.New(td.ctx, manager, td.blockHooks, td.updateHooks, td.responseAssembler) + return queryexecutor.New(td.ctx, manager, td.blockHooks, td.updateHooks) } func (td *testData) assertPausedRequest() { diff --git a/responsemanager/server.go b/responsemanager/server.go index 046dc20f..5460a798 100644 --- a/responsemanager/server.go +++ b/responsemanager/server.go @@ -79,7 +79,7 @@ func (rm *ResponseManager) processUpdate(key responseKey, update gsmsg.GraphSync return } // else this is a paused response, so the update needs to be handled here and not in the executor result := rm.updateHooks.ProcessUpdateHooks(key.p, response.request, update) - _ = rm.responseAssembler.Transaction(key.p, key.requestID, func(rb responseassembler.ResponseBuilder) error { + _ = response.responseStream.Transaction(func(rb responseassembler.ResponseBuilder) error { for _, extension := range result.Extensions { rb.SendExtensionData(extension) } @@ -116,7 +116,7 @@ func (rm *ResponseManager) unpauseRequest(p peer.ID, requestID graphsync.Request } inProgressResponse.state = graphsync.Queued if len(extensions) > 0 { - _ = rm.responseAssembler.Transaction(p, requestID, func(rb responseassembler.ResponseBuilder) error { + _ = inProgressResponse.responseStream.Transaction(func(rb responseassembler.ResponseBuilder) error { for _, extension := range extensions { rb.SendExtensionData(extension) } @@ -143,7 +143,7 @@ func (rm *ResponseManager) abortRequest(p peer.ID, requestID graphsync.RequestID response.span.SetStatus(codes.Error, err.Error()) if response.state != graphsync.Running { - _ = rm.responseAssembler.Transaction(p, requestID, func(rb responseassembler.ResponseBuilder) error { + _ = response.responseStream.Transaction(func(rb responseassembler.ResponseBuilder) error { if ipldutil.IsContextCancelErr(err) { rm.cancelledListeners.NotifyCancelledListeners(p, response.request) rb.ClearRequest() @@ -214,8 +214,9 @@ func (rm *ResponseManager) processRequests(p peer.ID, requests []gsmsg.GraphSync UpdateSignal: make(chan struct{}, 1), ErrSignal: make(chan error, 1), }, - state: graphsync.Queued, - startTime: time.Now(), + state: graphsync.Queued, + startTime: time.Now(), + responseStream: rm.responseAssembler.NewStream(key.p, key.requestID), } // TODO: Use a better work estimation metric. @@ -231,7 +232,7 @@ func (rm *ResponseManager) taskDataForKey(key responseKey) queryexecutor.Respons log.Infow("graphsync response processing begins", "request id", key.requestID, "peer", key.p, "total time", time.Since(response.startTime)) if response.loader == nil || response.traverser == nil { - loader, traverser, isPaused, err := (&queryPreparer{rm.requestHooks, rm.responseAssembler, rm.linkSystem, rm.maxLinksPerRequest}).prepareQuery(response.ctx, key.p, response.request, response.signals, response.subscriber) + loader, traverser, isPaused, err := (&queryPreparer{rm.requestHooks, rm.linkSystem, rm.maxLinksPerRequest}).prepareQuery(response.ctx, key.p, response.request, response.responseStream, response.signals, response.subscriber) if err != nil { response.state = graphsync.CompletingSend response.span.RecordError(err) @@ -247,14 +248,15 @@ func (rm *ResponseManager) taskDataForKey(key responseKey) queryexecutor.Respons } response.state = graphsync.Running return queryexecutor.ResponseTask{ - Ctx: response.ctx, - Span: response.span, - Empty: false, - Subscriber: response.subscriber, - Request: response.request, - Loader: response.loader, - Traverser: response.traverser, - Signals: response.signals, + Ctx: response.ctx, + Span: response.span, + Empty: false, + Subscriber: response.subscriber, + Request: response.request, + Loader: response.loader, + Traverser: response.traverser, + Signals: response.signals, + ResponseStream: response.responseStream, } } From 9f76c865455ae7c8661e40e98d19608f66d26e12 Mon Sep 17 00:00:00 2001 From: hannahhoward Date: Tue, 14 Dec 2021 22:52:02 -0800 Subject: [PATCH 04/12] fix(messagequeue): remove unused message --- messagequeue/messagequeue.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/messagequeue/messagequeue.go b/messagequeue/messagequeue.go index 0c9cd980..71056643 100644 --- a/messagequeue/messagequeue.go +++ b/messagequeue/messagequeue.go @@ -352,7 +352,3 @@ func (mq *MessageQueue) publishError(metadata messageMetadata, err error) { mq.eventPublisher.Publish(metadata.topic, Event{Name: Error, Err: err}) _ = mq.allocator.ReleaseBlockMemory(mq.p, metadata.msgSize) } - -func (mq *MessageQueue) close(metadata messageMetadata) { - mq.eventPublisher.Close(metadata.topic) -} From 35d5ada279af0a0741349fc9753c7e664a7e274a Mon Sep 17 00:00:00 2001 From: hannahhoward Date: Tue, 14 Dec 2021 22:54:40 -0800 Subject: [PATCH 05/12] test(impl): add test to verify single abort message --- impl/graphsync_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/impl/graphsync_test.go b/impl/graphsync_test.go index bb977e26..87011d47 100644 --- a/impl/graphsync_test.go +++ b/impl/graphsync_test.go @@ -980,6 +980,8 @@ func TestNetworkDisconnect(t *testing.T) { tracing := collectTracing(t) traceStrings := tracing.TracesToStrings() require.Contains(t, traceStrings, "response(0)->executeTask(0)") + require.Contains(t, traceStrings, "response(0)->abortRequest(0)") + require.NotContains(t, traceStrings, "response(0)->abortRequest(1)") // may contain multiple abortRequest traces as the network error can bubble up >1 times // but these will only record if the request is still executing require.Contains(t, traceStrings, "request(0)->newRequest(0)") From f4c8fbf62c134b79f1895311710f2c7f83dbf578 Mon Sep 17 00:00:00 2001 From: hannahhoward Date: Wed, 15 Dec 2021 17:57:20 -0800 Subject: [PATCH 06/12] feat(responseassembler): refactor notifications --- benchmarks/testnet/network_test.go | 4 +- impl/graphsync_test.go | 2 +- message/builder.go | 27 +--- message/builder_test.go | 7 +- message/message.go | 8 ++ message/message_test.go | 12 +- messagequeue/builder.go | 89 ++++++++++++ messagequeue/messagequeue.go | 61 ++++---- messagequeue/messagequeue_test.go | 112 +++++++++------ network/libp2p_impl_test.go | 2 +- peermanager/peermessagemanager.go | 10 +- peermanager/peermessagemanager_test.go | 18 +-- requestmanager/client.go | 14 +- requestmanager/requestmanager_test.go | 6 +- responsemanager/client.go | 3 +- .../queryexecutor/queryexecutor.go | 44 +++--- .../queryexecutor/queryexecutor_test.go | 57 ++------ responsemanager/querypreparer.go | 23 +-- .../responseassembler/responseBuilder.go | 21 +-- .../responseassembler/responseassembler.go | 32 +++-- .../responseassembler_test.go | 135 ++++++++++++------ responsemanager/responsemanager_test.go | 108 ++++++++------ responsemanager/server.go | 48 +++---- responsemanager/subscriber.go | 36 +++-- testutil/testnotifications.go | 64 ++++----- 25 files changed, 511 insertions(+), 432 deletions(-) create mode 100644 messagequeue/builder.go diff --git a/benchmarks/testnet/network_test.go b/benchmarks/testnet/network_test.go index 5fd79917..424c0679 100644 --- a/benchmarks/testnet/network_test.go +++ b/benchmarks/testnet/network_test.go @@ -33,7 +33,7 @@ func TestSendMessageAsyncButWaitForResponse(t *testing.T) { fromWaiter peer.ID, msgFromWaiter gsmsg.GraphSyncMessage) { - builder := gsmsg.NewBuilder(gsmsg.Topic(0)) + builder := gsmsg.NewBuilder() builder.AddBlock(blocks.NewBlock([]byte(expectedStr))) msgToWaiter, err := builder.Build() require.NoError(t, err) @@ -62,7 +62,7 @@ func TestSendMessageAsyncButWaitForResponse(t *testing.T) { } })) - builder := gsmsg.NewBuilder(gsmsg.Topic(0)) + builder := gsmsg.NewBuilder() builder.AddBlock(blocks.NewBlock([]byte("data"))) messageSentAsync, err := builder.Build() require.NoError(t, err) diff --git a/impl/graphsync_test.go b/impl/graphsync_test.go index 87011d47..b85b4553 100644 --- a/impl/graphsync_test.go +++ b/impl/graphsync_test.go @@ -135,7 +135,7 @@ func TestSendResponseToIncomingRequest(t *testing.T) { requestID := graphsync.RequestID(rand.Int31()) - builder := gsmsg.NewBuilder(gsmsg.Topic(0)) + builder := gsmsg.NewBuilder() builder.AddRequest(gsmsg.NewRequest(requestID, blockChain.TipLink.(cidlink.Link).Cid, blockChain.Selector(), graphsync.Priority(math.MaxInt32), td.extension)) message, err := builder.Build() require.NoError(t, err) diff --git a/message/builder.go b/message/builder.go index 6914f08a..15017198 100644 --- a/message/builder.go +++ b/message/builder.go @@ -1,8 +1,6 @@ package message import ( - "io" - blocks "github.com/ipfs/go-block-format" "github.com/ipfs/go-cid" "github.com/ipld/go-ipld-prime" @@ -16,42 +14,25 @@ import ( // requests for a given peer and then generates the corresponding // GraphSync message when ready to send type Builder struct { - topic Topic outgoingBlocks map[cid.Cid]blocks.Block blkSize uint64 completedResponses map[graphsync.RequestID]graphsync.ResponseStatusCode outgoingResponses map[graphsync.RequestID]metadata.Metadata extensions map[graphsync.RequestID][]graphsync.ExtensionData requests map[graphsync.RequestID]GraphSyncRequest - responseStreams map[graphsync.RequestID]io.Closer } -// Topic is an identifier for notifications about this response builder -type Topic uint64 - // NewBuilder generates a new Builder. -func NewBuilder(topic Topic) *Builder { +func NewBuilder() *Builder { return &Builder{ - topic: topic, requests: make(map[graphsync.RequestID]GraphSyncRequest), outgoingBlocks: make(map[cid.Cid]blocks.Block), completedResponses: make(map[graphsync.RequestID]graphsync.ResponseStatusCode), outgoingResponses: make(map[graphsync.RequestID]metadata.Metadata), extensions: make(map[graphsync.RequestID][]graphsync.ExtensionData), - responseStreams: make(map[graphsync.RequestID]io.Closer), } } -// AddResponseStream adds a stream that can be closed if the message fails to send -func (b *Builder) AddResponseStream(requestID graphsync.RequestID, responseStream io.Closer) { - b.responseStreams[requestID] = responseStream -} - -// ResponseStreams returns related response streams for a given builder -func (b *Builder) ResponseStreams() map[graphsync.RequestID]io.Closer { - return b.responseStreams -} - // AddRequest registers a new request to be added to the message. func (b *Builder) AddRequest(request GraphSyncRequest) { b.requests[request.ID()] = request @@ -107,7 +88,6 @@ func (b *Builder) ScrubResponses(requestIDs []graphsync.RequestID) uint64 { delete(b.completedResponses, requestID) delete(b.extensions, requestID) delete(b.outgoingResponses, requestID) - delete(b.responseStreams, requestID) } oldSize := b.blkSize newBlkSize := uint64(0) @@ -147,11 +127,6 @@ func (b *Builder) Build() (GraphSyncMessage, error) { }, nil } -// Topic returns the identifier for notifications sent about this builder -func (b *Builder) Topic() Topic { - return b.topic -} - func responseCode(status graphsync.ResponseStatusCode, isComplete bool) graphsync.ResponseStatusCode { if !isComplete { return graphsync.PartialResponse diff --git a/message/builder_test.go b/message/builder_test.go index 2995636b..b9722c84 100644 --- a/message/builder_test.go +++ b/message/builder_test.go @@ -67,8 +67,6 @@ func TestMessageBuilding(t *testing.T) { for _, block := range blocks { rb.AddBlock(block) } - rb.AddResponseStream(requestID1, closer) - rb.AddResponseStream(requestID2, closer) }, expectedSize: 300, expectedStreams: map[graphsync.RequestID]io.Closer{ @@ -241,12 +239,9 @@ func TestMessageBuilding(t *testing.T) { } for testCase, data := range testCases { t.Run(testCase, func(t *testing.T) { - b := NewBuilder(Topic(rand.Uint64())) + b := NewBuilder() data.build(b) require.Equal(t, data.expectedSize, b.BlockSize(), "did not calculate block size correctly") - if data.expectedStreams != nil { - require.Equal(t, data.expectedStreams, b.ResponseStreams()) - } message, err := b.Build() require.NoError(t, err, "build message errored") data.checkMsg(t, message) diff --git a/message/message.go b/message/message.go index adf7d733..6e818847 100644 --- a/message/message.go +++ b/message/message.go @@ -218,6 +218,14 @@ func (gsm GraphSyncMessage) Requests() []GraphSyncRequest { return requests } +func (gsm GraphSyncMessage) ResponseCodes() map[graphsync.RequestID]graphsync.ResponseStatusCode { + codes := make(map[graphsync.RequestID]graphsync.ResponseStatusCode, len(gsm.responses)) + for id, response := range gsm.responses { + codes[id] = response.Status() + } + return codes +} + func (gsm GraphSyncMessage) Responses() []GraphSyncResponse { responses := make([]GraphSyncResponse, 0, len(gsm.responses)) for _, response := range gsm.responses { diff --git a/message/message_test.go b/message/message_test.go index c69a92fd..135342d3 100644 --- a/message/message_test.go +++ b/message/message_test.go @@ -29,7 +29,7 @@ func TestAppendingRequests(t *testing.T) { id := graphsync.RequestID(rand.Int31()) priority := graphsync.Priority(rand.Int31()) - builder := NewBuilder(Topic(0)) + builder := NewBuilder() builder.AddRequest(NewRequest(id, root, selector, priority, extension)) gsm, err := builder.Build() require.NoError(t, err) @@ -85,7 +85,7 @@ func TestAppendingResponses(t *testing.T) { requestID := graphsync.RequestID(rand.Int31()) status := graphsync.RequestAcknowledged - builder := NewBuilder(Topic(0)) + builder := NewBuilder() builder.AddResponseCode(requestID, status) builder.AddExtensionData(requestID, extension) gsm, err := builder.Build() @@ -124,7 +124,7 @@ func TestAppendBlock(t *testing.T) { strs = append(strs, "Celeritas") strs = append(strs, "Incendia") - builder := NewBuilder(Topic(0)) + builder := NewBuilder() for _, str := range strs { block := blocks.NewBlock([]byte(str)) builder.AddBlock(block) @@ -158,7 +158,7 @@ func TestRequestCancel(t *testing.T) { priority := graphsync.Priority(rand.Int31()) root := testutil.GenerateCids(1)[0] - builder := NewBuilder(Topic(0)) + builder := NewBuilder() builder.AddRequest(NewRequest(id, root, selector, priority)) builder.AddRequest(CancelRequest(id)) gsm, err := builder.Build() @@ -191,7 +191,7 @@ func TestRequestUpdate(t *testing.T) { Data: testutil.RandomBytes(100), } - builder := NewBuilder(Topic(0)) + builder := NewBuilder() builder.AddRequest(UpdateRequest(id, extension)) gsm, err := builder.Build() require.NoError(t, err) @@ -239,7 +239,7 @@ func TestToNetFromNetEquivalency(t *testing.T) { priority := graphsync.Priority(rand.Int31()) status := graphsync.RequestAcknowledged - builder := NewBuilder(Topic(0)) + builder := NewBuilder() builder.AddRequest(NewRequest(id, root, selector, priority, extension)) builder.AddResponseCode(id, status) builder.AddExtensionData(id, extension) diff --git a/messagequeue/builder.go b/messagequeue/builder.go new file mode 100644 index 00000000..fb794310 --- /dev/null +++ b/messagequeue/builder.go @@ -0,0 +1,89 @@ +package messagequeue + +import ( + "io" + + "github.com/ipfs/go-graphsync" + gsmsg "github.com/ipfs/go-graphsync/message" + "github.com/ipfs/go-graphsync/notifications" +) + +// Builder wraps a message builder with additional functions related to metadata +// and notifications in the message queue +type Builder struct { + *gsmsg.Builder + topic Topic + responseStreams map[graphsync.RequestID]io.Closer + subscribers map[graphsync.RequestID]notifications.Subscriber + blockData map[graphsync.RequestID][]graphsync.BlockData +} + +// NewBuilder sets up a new builder for the given topic +func NewBuilder(topic Topic) *Builder { + return &Builder{ + Builder: gsmsg.NewBuilder(), + topic: topic, + responseStreams: make(map[graphsync.RequestID]io.Closer), + subscribers: make(map[graphsync.RequestID]notifications.Subscriber), + blockData: make(map[graphsync.RequestID][]graphsync.BlockData), + } +} + +// SetResponseStream sets the given response stream to close should the message fail to send +func (b *Builder) SetResponseStream(requestID graphsync.RequestID, stream io.Closer) { + b.responseStreams[requestID] = stream +} + +// SetSubscriber sets the given subscriber to get notified as events occur for this message +func (b *Builder) SetSubscriber(requestID graphsync.RequestID, subscriber notifications.Subscriber) { + b.subscribers[requestID] = subscriber +} + +// AddBlockData add the given block metadata for this message to pass into notifications +func (b *Builder) AddBlockData(requestID graphsync.RequestID, blockData graphsync.BlockData) { + b.blockData[requestID] = append(b.blockData[requestID], blockData) +} + +// ScrubResponse removes the given responses from the message and metadata +func (b *Builder) ScrubResponses(requestIDs []graphsync.RequestID) uint64 { + for _, requestID := range requestIDs { + delete(b.responseStreams, requestID) + delete(b.subscribers, requestID) + delete(b.blockData, requestID) + } + return b.Builder.ScrubResponses(requestIDs) +} + +// ResponseStreams inspect current response stream state +func (b *Builder) ResponseStreams() map[graphsync.RequestID]io.Closer { + return b.responseStreams +} + +// Subscribers inspect current subscribers +func (b *Builder) Subscribers() map[graphsync.RequestID]notifications.Subscriber { + return b.subscribers +} + +// BlockData inspects current block data +func (b *Builder) BlockData() map[graphsync.RequestID][]graphsync.BlockData { + return b.blockData +} + +func (b *Builder) build(publisher notifications.Publisher) (gsmsg.GraphSyncMessage, internalMetadata, error) { + message, err := b.Build() + if err != nil { + return gsmsg.GraphSyncMessage{}, internalMetadata{}, err + } + for _, subscriber := range b.subscribers { + publisher.Subscribe(b.topic, subscriber) + } + return message, internalMetadata{ + public: Metadata{ + BlockData: b.blockData, + ResponseCodes: message.ResponseCodes(), + }, + topic: b.topic, + msgSize: b.BlockSize(), + responseStreams: b.responseStreams, + }, nil +} diff --git a/messagequeue/messagequeue.go b/messagequeue/messagequeue.go index 71056643..cf3c02a3 100644 --- a/messagequeue/messagequeue.go +++ b/messagequeue/messagequeue.go @@ -22,6 +22,8 @@ var log = logging.Logger("graphsync") // max block size is the maximum size for batching blocks in a single payload const maxBlockSize uint64 = 512 * 1024 +type Topic uint64 + type EventName uint64 const ( @@ -30,9 +32,15 @@ const ( Error ) +type Metadata struct { + BlockData map[graphsync.RequestID][]graphsync.BlockData + ResponseCodes map[graphsync.RequestID]graphsync.ResponseStatusCode +} + type Event struct { - Name EventName - Err error + Name EventName + Err error + Metadata Metadata } // MessageNetwork is any network that can connect peers and generate a message @@ -61,8 +69,8 @@ type MessageQueue struct { sender gsnet.MessageSender eventPublisher notifications.Publisher buildersLk sync.RWMutex - builders []*gsmsg.Builder - nextBuilderTopic gsmsg.Topic + builders []*Builder + nextBuilderTopic Topic allocator Allocator maxRetries int sendMessageTimeout time.Duration @@ -85,7 +93,7 @@ func New(ctx context.Context, p peer.ID, network MessageNetwork, allocator Alloc // AllocateAndBuildMessage allows you to work modify the next message that is sent in the queue. // If blkSize > 0, message building may block until enough memory has been freed from the queues to allocate the message. -func (mq *MessageQueue) AllocateAndBuildMessage(size uint64, buildMessageFn func(*gsmsg.Builder), notifees []notifications.Notifee) { +func (mq *MessageQueue) AllocateAndBuildMessage(size uint64, buildMessageFn func(*Builder)) { if size > 0 { select { case <-mq.allocator.AllocateBlockMemory(mq.p, size): @@ -93,28 +101,25 @@ func (mq *MessageQueue) AllocateAndBuildMessage(size uint64, buildMessageFn func return } } - if mq.buildMessage(size, buildMessageFn, notifees) { + if mq.buildMessage(size, buildMessageFn) { mq.signalWork() } } -func (mq *MessageQueue) buildMessage(size uint64, buildMessageFn func(*gsmsg.Builder), notifees []notifications.Notifee) bool { +func (mq *MessageQueue) buildMessage(size uint64, buildMessageFn func(*Builder)) bool { mq.buildersLk.Lock() defer mq.buildersLk.Unlock() if shouldBeginNewResponse(mq.builders, size) { topic := mq.nextBuilderTopic mq.nextBuilderTopic++ - mq.builders = append(mq.builders, gsmsg.NewBuilder(topic)) + mq.builders = append(mq.builders, NewBuilder(topic)) } builder := mq.builders[len(mq.builders)-1] buildMessageFn(builder) - for _, notifee := range notifees { - notifications.SubscribeWithData(mq.eventPublisher, builder.Topic(), notifee) - } return !builder.Empty() } -func shouldBeginNewResponse(builders []*gsmsg.Builder, blkSize uint64) bool { +func shouldBeginNewResponse(builders []*Builder, blkSize uint64) bool { if len(builders) == 0 { return true } @@ -181,12 +186,12 @@ func (mq *MessageQueue) signalWork() { var errEmptyMessage = errors.New("empty Message") -func (mq *MessageQueue) extractOutgoingMessage() (gsmsg.GraphSyncMessage, messageMetadata, error) { +func (mq *MessageQueue) extractOutgoingMessage() (gsmsg.GraphSyncMessage, internalMetadata, error) { // grab outgoing message mq.buildersLk.Lock() if len(mq.builders) == 0 { mq.buildersLk.Unlock() - return gsmsg.GraphSyncMessage{}, messageMetadata{}, errEmptyMessage + return gsmsg.GraphSyncMessage{}, internalMetadata{}, errEmptyMessage } builder := mq.builders[0] mq.builders = mq.builders[1:] @@ -199,10 +204,9 @@ func (mq *MessageQueue) extractOutgoingMessage() (gsmsg.GraphSyncMessage, messag } mq.buildersLk.Unlock() if builder.Empty() { - return gsmsg.GraphSyncMessage{}, messageMetadata{}, errEmptyMessage + return gsmsg.GraphSyncMessage{}, internalMetadata{}, errEmptyMessage } - message, err := builder.Build() - return message, messageMetadata{builder.Topic(), builder.ResponseStreams(), builder.BlockSize()}, err + return builder.build(mq.eventPublisher) } func (mq *MessageQueue) sendMessage() { @@ -251,7 +255,7 @@ func (mq *MessageQueue) scrubResponseStreams(responseStreams map[graphsync.Reque // from all pending messages in the queue func (mq *MessageQueue) scrubResponses(requestIDs []graphsync.RequestID) uint64 { mq.buildersLk.Lock() - newBuilders := make([]*gsmsg.Builder, 0, len(mq.builders)) + newBuilders := make([]*Builder, 0, len(mq.builders)) totalFreed := uint64(0) for _, builder := range mq.builders { totalFreed = builder.ScrubResponses(requestIDs) @@ -276,7 +280,7 @@ func (mq *MessageQueue) initializeSender() error { return nil } -func (mq *MessageQueue) attemptSendAndRecovery(message gsmsg.GraphSyncMessage, metadata messageMetadata) bool { +func (mq *MessageQueue) attemptSendAndRecovery(message gsmsg.GraphSyncMessage, metadata internalMetadata) bool { err := mq.sender.SendMsg(mq.ctx, message) if err == nil { mq.publishSent(metadata) @@ -332,23 +336,24 @@ func openSender(ctx context.Context, network MessageNetwork, p peer.ID, sendTime return nsender, nil } -type messageMetadata struct { - topic gsmsg.Topic - responseStreams map[graphsync.RequestID]io.Closer +type internalMetadata struct { + public Metadata + topic Topic msgSize uint64 + responseStreams map[graphsync.RequestID]io.Closer } -func (mq *MessageQueue) publishQueued(metadata messageMetadata) { - mq.eventPublisher.Publish(metadata.topic, Event{Name: Queued}) +func (mq *MessageQueue) publishQueued(metadata internalMetadata) { + mq.eventPublisher.Publish(metadata.topic, Event{Name: Queued, Metadata: metadata.public}) } -func (mq *MessageQueue) publishSent(metadata messageMetadata) { - mq.eventPublisher.Publish(metadata.topic, Event{Name: Sent}) +func (mq *MessageQueue) publishSent(metadata internalMetadata) { + mq.eventPublisher.Publish(metadata.topic, Event{Name: Sent, Metadata: metadata.public}) _ = mq.allocator.ReleaseBlockMemory(mq.p, metadata.msgSize) } -func (mq *MessageQueue) publishError(metadata messageMetadata, err error) { +func (mq *MessageQueue) publishError(metadata internalMetadata, err error) { mq.scrubResponseStreams(metadata.responseStreams) - mq.eventPublisher.Publish(metadata.topic, Event{Name: Error, Err: err}) + mq.eventPublisher.Publish(metadata.topic, Event{Name: Error, Err: err, Metadata: metadata.public}) _ = mq.allocator.ReleaseBlockMemory(mq.p, metadata.msgSize) } diff --git a/messagequeue/messagequeue_test.go b/messagequeue/messagequeue_test.go index a991b721..7358e0f4 100644 --- a/messagequeue/messagequeue_test.go +++ b/messagequeue/messagequeue_test.go @@ -46,9 +46,9 @@ func TestStartupAndShutdown(t *testing.T) { root := testutil.GenerateCids(1)[0] waitGroup.Add(1) - messageQueue.AllocateAndBuildMessage(0, func(b *gsmsg.Builder) { + messageQueue.AllocateAndBuildMessage(0, func(b *Builder) { b.AddRequest(gsmsg.NewRequest(id, root, selector, priority)) - }, []notifications.Notifee{}) + }) testutil.AssertDoesReceive(ctx, t, messagesSent, "message was not sent") @@ -85,9 +85,9 @@ func TestShutdownDuringMessageSend(t *testing.T) { // setup a message and advance as far as beginning to send it waitGroup.Add(1) - messageQueue.AllocateAndBuildMessage(0, func(b *gsmsg.Builder) { + messageQueue.AllocateAndBuildMessage(0, func(b *Builder) { b.AddRequest(gsmsg.NewRequest(id, root, selector, priority)) - }, []notifications.Notifee{}) + }) waitGroup.Wait() // now shut down @@ -135,12 +135,14 @@ func TestProcessingNotification(t *testing.T) { Data: testutil.RandomBytes(100), } status := graphsync.RequestCompletedFull - expectedTopic := "testTopic" - notifee, verifier := testutil.NewTestNotifee(expectedTopic, 5) - messageQueue.AllocateAndBuildMessage(0, func(b *gsmsg.Builder) { + blkData := testutil.NewFakeBlockData() + subscriber := testutil.NewTestSubscriber(5) + messageQueue.AllocateAndBuildMessage(0, func(b *Builder) { b.AddResponseCode(responseID, status) b.AddExtensionData(responseID, extension) - }, []notifications.Notifee{notifee}) + b.AddBlockData(responseID, blkData) + b.SetSubscriber(responseID, subscriber) + }) // wait for send attempt waitGroup.Wait() @@ -158,11 +160,25 @@ func TestProcessingNotification(t *testing.T) { require.True(t, found) require.Equal(t, extension.Data, extensionData) - verifier.ExpectEvents(ctx, t, []notifications.Event{ - Event{Name: Queued}, - Event{Name: Sent}, + expectedMetadata := Metadata{ + ResponseCodes: map[graphsync.RequestID]graphsync.ResponseStatusCode{ + responseID: status, + }, + BlockData: map[graphsync.RequestID][]graphsync.BlockData{ + responseID: []graphsync.BlockData{blkData}, + }, + } + subscriber.ExpectEventsAllTopics(ctx, t, []notifications.Event{ + Event{ + Name: Queued, + Metadata: expectedMetadata, + }, + Event{ + Name: Sent, + Metadata: expectedMetadata, + }, }) - verifier.ExpectClose(ctx, t) + subscriber.ExpectNCloses(ctx, t, 1) } func TestDedupingMessages(t *testing.T) { @@ -188,9 +204,9 @@ func TestDedupingMessages(t *testing.T) { selector := ssb.Matcher().Node() root := testutil.GenerateCids(1)[0] - messageQueue.AllocateAndBuildMessage(0, func(b *gsmsg.Builder) { + messageQueue.AllocateAndBuildMessage(0, func(b *Builder) { b.AddRequest(gsmsg.NewRequest(id, root, selector, priority)) - }, []notifications.Notifee{}) + }) // wait for send attempt waitGroup.Wait() id2 := graphsync.RequestID(rand.Int31()) @@ -202,10 +218,10 @@ func TestDedupingMessages(t *testing.T) { selector3 := ssb.ExploreIndex(0, ssb.Matcher()).Node() root3 := testutil.GenerateCids(1)[0] - messageQueue.AllocateAndBuildMessage(0, func(b *gsmsg.Builder) { + messageQueue.AllocateAndBuildMessage(0, func(b *Builder) { b.AddRequest(gsmsg.NewRequest(id2, root2, selector2, priority2)) b.AddRequest(gsmsg.NewRequest(id3, root3, selector3, priority3)) - }, []notifications.Notifee{}) + }) var message gsmsg.GraphSyncMessage testutil.AssertReceive(ctx, t, messagesSent, &message, "message did not send") @@ -257,9 +273,9 @@ func TestSendsVeryLargeBlocksResponses(t *testing.T) { // generate large blocks before proceeding blks := testutil.GenerateBlocksOfSize(5, 1000000) - messageQueue.AllocateAndBuildMessage(uint64(len(blks[0].RawData())), func(b *gsmsg.Builder) { + messageQueue.AllocateAndBuildMessage(uint64(len(blks[0].RawData())), func(b *Builder) { b.AddBlock(blks[0]) - }, []notifications.Notifee{}) + }) waitGroup.Wait() var message gsmsg.GraphSyncMessage testutil.AssertReceive(ctx, t, messagesSent, &message, "message did not send") @@ -269,15 +285,15 @@ func TestSendsVeryLargeBlocksResponses(t *testing.T) { require.True(t, blks[0].Cid().Equals(msgBlks[0].Cid())) // Send 3 very large blocks - messageQueue.AllocateAndBuildMessage(uint64(len(blks[1].RawData())), func(b *gsmsg.Builder) { + messageQueue.AllocateAndBuildMessage(uint64(len(blks[1].RawData())), func(b *Builder) { b.AddBlock(blks[1]) - }, []notifications.Notifee{}) - messageQueue.AllocateAndBuildMessage(uint64(len(blks[2].RawData())), func(b *gsmsg.Builder) { + }) + messageQueue.AllocateAndBuildMessage(uint64(len(blks[2].RawData())), func(b *Builder) { b.AddBlock(blks[2]) - }, []notifications.Notifee{}) - messageQueue.AllocateAndBuildMessage(uint64(len(blks[3].RawData())), func(b *gsmsg.Builder) { + }) + messageQueue.AllocateAndBuildMessage(uint64(len(blks[3].RawData())), func(b *Builder) { b.AddBlock(blks[3]) - }, []notifications.Notifee{}) + }) testutil.AssertReceive(ctx, t, messagesSent, &message, "message did not send") msgBlks = message.Blocks() @@ -317,16 +333,16 @@ func TestSendsResponsesMemoryPressure(t *testing.T) { // start sending block that exceeds memory limit blks := testutil.GenerateBlocksOfSize(2, 999) - messageQueue.AllocateAndBuildMessage(uint64(len(blks[0].RawData())), func(b *gsmsg.Builder) { + messageQueue.AllocateAndBuildMessage(uint64(len(blks[0].RawData())), func(b *Builder) { b.AddBlock(blks[0]) - }, []notifications.Notifee{}) + }) finishes := make(chan string, 2) go func() { // attempt to send second block. Should block until memory is released - messageQueue.AllocateAndBuildMessage(uint64(len(blks[1].RawData())), func(b *gsmsg.Builder) { + messageQueue.AllocateAndBuildMessage(uint64(len(blks[1].RawData())), func(b *Builder) { b.AddBlock(blks[1]) - }, []notifications.Notifee{}) + }) finishes <- "sent message" }() @@ -376,13 +392,13 @@ func TestNetworkErrorClearResponses(t *testing.T) { // generate large blocks before proceeding blks := testutil.GenerateBlocksOfSize(5, 1000000) - expectedTopic := "testTopic" - notifee, verifier := testutil.NewTestNotifee(expectedTopic, 5) + subscriber := testutil.NewTestSubscriber(5) - messageQueue.AllocateAndBuildMessage(uint64(len(blks[0].RawData())), func(b *gsmsg.Builder) { + messageQueue.AllocateAndBuildMessage(uint64(len(blks[0].RawData())), func(b *Builder) { b.AddBlock(blks[0]) b.AddLink(requestID1, cidlink.Link{Cid: blks[0].Cid()}, true) - }, []notifications.Notifee{notifee}) + b.SetSubscriber(requestID1, subscriber) + }) waitGroup.Wait() var message gsmsg.GraphSyncMessage testutil.AssertReceive(ctx, t, messagesSent, &message, "message did not send") @@ -391,29 +407,35 @@ func TestNetworkErrorClearResponses(t *testing.T) { require.Len(t, msgBlks, 1, "number of blks in first message was not 1") require.True(t, blks[0].Cid().Equals(msgBlks[0].Cid())) - verifier.ExpectEvents(ctx, t, []notifications.Event{ - Event{Name: Queued}, - Event{Name: Sent}, + expectedMetadata := Metadata{ + ResponseCodes: map[graphsync.RequestID]graphsync.ResponseStatusCode{ + requestID1: graphsync.PartialResponse, + }, + BlockData: map[graphsync.RequestID][]graphsync.BlockData{}, + } + subscriber.ExpectEventsAllTopics(ctx, t, []notifications.Event{ + Event{Name: Queued, Metadata: expectedMetadata}, + Event{Name: Sent, Metadata: expectedMetadata}, }) - verifier.ExpectClose(ctx, t) + subscriber.ExpectNCloses(ctx, t, 1) fc1 := &fakeCloser{fms: messageSender} fc2 := &fakeCloser{fms: messageSender} // Send 3 very large blocks - messageQueue.AllocateAndBuildMessage(uint64(len(blks[1].RawData())), func(b *gsmsg.Builder) { + messageQueue.AllocateAndBuildMessage(uint64(len(blks[1].RawData())), func(b *Builder) { b.AddBlock(blks[1]) - b.AddResponseStream(requestID1, fc1) + b.SetResponseStream(requestID1, fc1) b.AddLink(requestID1, cidlink.Link{Cid: blks[1].Cid()}, true) - }, []notifications.Notifee{}) - messageQueue.AllocateAndBuildMessage(uint64(len(blks[2].RawData())), func(b *gsmsg.Builder) { + }) + messageQueue.AllocateAndBuildMessage(uint64(len(blks[2].RawData())), func(b *Builder) { b.AddBlock(blks[2]) - b.AddResponseStream(requestID1, fc1) + b.SetResponseStream(requestID1, fc1) b.AddLink(requestID1, cidlink.Link{Cid: blks[2].Cid()}, true) - }, []notifications.Notifee{}) - messageQueue.AllocateAndBuildMessage(uint64(len(blks[3].RawData())), func(b *gsmsg.Builder) { - b.AddResponseStream(requestID2, fc2) + }) + messageQueue.AllocateAndBuildMessage(uint64(len(blks[3].RawData())), func(b *Builder) { + b.SetResponseStream(requestID2, fc2) b.AddLink(requestID2, cidlink.Link{Cid: blks[3].Cid()}, true) b.AddBlock(blks[3]) - }, []notifications.Notifee{}) + }) messageSender.sendError = errors.New("something went wrong") diff --git a/network/libp2p_impl_test.go b/network/libp2p_impl_test.go index 76dc8b56..da7d2225 100644 --- a/network/libp2p_impl_test.go +++ b/network/libp2p_impl_test.go @@ -81,7 +81,7 @@ func TestMessageSendAndReceive(t *testing.T) { priority := graphsync.Priority(rand.Int31()) status := graphsync.RequestAcknowledged - builder := gsmsg.NewBuilder(gsmsg.Topic(0)) + builder := gsmsg.NewBuilder() builder.AddRequest(gsmsg.NewRequest(id, root, selector, priority)) builder.AddResponseCode(id, status) builder.AddExtensionData(id, extension) diff --git a/peermanager/peermessagemanager.go b/peermanager/peermessagemanager.go index 45aa89bc..fef59501 100644 --- a/peermanager/peermessagemanager.go +++ b/peermanager/peermessagemanager.go @@ -3,16 +3,14 @@ package peermanager import ( "context" + "github.com/ipfs/go-graphsync/messagequeue" "github.com/libp2p/go-libp2p-core/peer" - - gsmsg "github.com/ipfs/go-graphsync/message" - "github.com/ipfs/go-graphsync/notifications" ) // PeerQueue is a process that sends messages to a peer type PeerQueue interface { PeerProcess - AllocateAndBuildMessage(blkSize uint64, buildMessageFn func(*gsmsg.Builder), notifees []notifications.Notifee) + AllocateAndBuildMessage(blkSize uint64, buildMessageFn func(*messagequeue.Builder)) } // PeerQueueFactory provides a function that will create a PeerQueue. @@ -34,7 +32,7 @@ func NewMessageManager(ctx context.Context, createPeerQueue PeerQueueFactory) *P // BuildMessage allows you to modify the next message that is sent for the given peer // If blkSize > 0, message building may block until enough memory has been freed from the queues to allocate the message. -func (pmm *PeerMessageManager) AllocateAndBuildMessage(p peer.ID, blkSize uint64, buildMessageFn func(*gsmsg.Builder), notifees []notifications.Notifee) { +func (pmm *PeerMessageManager) AllocateAndBuildMessage(p peer.ID, blkSize uint64, buildMessageFn func(*messagequeue.Builder)) { pq := pmm.GetProcess(p).(PeerQueue) - pq.AllocateAndBuildMessage(blkSize, buildMessageFn, notifees) + pq.AllocateAndBuildMessage(blkSize, buildMessageFn) } diff --git a/peermanager/peermessagemanager_test.go b/peermanager/peermessagemanager_test.go index 03b84ec1..34f728bc 100644 --- a/peermanager/peermessagemanager_test.go +++ b/peermanager/peermessagemanager_test.go @@ -13,7 +13,7 @@ import ( "github.com/ipfs/go-graphsync" gsmsg "github.com/ipfs/go-graphsync/message" - "github.com/ipfs/go-graphsync/notifications" + "github.com/ipfs/go-graphsync/messagequeue" "github.com/ipfs/go-graphsync/testutil" ) @@ -29,8 +29,8 @@ type fakePeer struct { messagesSent chan messageSent } -func (fp *fakePeer) AllocateAndBuildMessage(blkSize uint64, buildMessage func(b *gsmsg.Builder), notifees []notifications.Notifee) { - builder := gsmsg.NewBuilder(gsmsg.Topic(0)) +func (fp *fakePeer) AllocateAndBuildMessage(blkSize uint64, buildMessage func(b *messagequeue.Builder)) { + builder := messagequeue.NewBuilder(messagequeue.Topic(0)) buildMessage(builder) message, err := builder.Build() if err != nil { @@ -76,16 +76,16 @@ func TestSendingMessagesToPeers(t *testing.T) { peerManager := NewMessageManager(ctx, peerQueueFactory) request := gsmsg.NewRequest(id, root, selector, priority) - peerManager.AllocateAndBuildMessage(tp[0], 0, func(b *gsmsg.Builder) { + peerManager.AllocateAndBuildMessage(tp[0], 0, func(b *messagequeue.Builder) { b.AddRequest(request) - }, []notifications.Notifee{}) - peerManager.AllocateAndBuildMessage(tp[1], 0, func(b *gsmsg.Builder) { + }) + peerManager.AllocateAndBuildMessage(tp[1], 0, func(b *messagequeue.Builder) { b.AddRequest(request) - }, []notifications.Notifee{}) + }) cancelRequest := gsmsg.CancelRequest(id) - peerManager.AllocateAndBuildMessage(tp[0], 0, func(b *gsmsg.Builder) { + peerManager.AllocateAndBuildMessage(tp[0], 0, func(b *messagequeue.Builder) { b.AddRequest(cancelRequest) - }, []notifications.Notifee{}) + }) var firstMessage messageSent testutil.AssertReceive(ctx, t, messagesSent, &firstMessage, "first message did not send") diff --git a/requestmanager/client.go b/requestmanager/client.go index 95081b3d..b0996c24 100644 --- a/requestmanager/client.go +++ b/requestmanager/client.go @@ -67,7 +67,7 @@ type inProgressRequestStatus struct { // PeerHandler is an interface that can send requests to peers type PeerHandler interface { - AllocateAndBuildMessage(p peer.ID, blkSize uint64, buildMessageFn func(*gsmsg.Builder), notifees []notifications.Notifee) + AllocateAndBuildMessage(p peer.ID, blkSize uint64, buildMessageFn func(*messagequeue.Builder)) } // AsyncLoader is an interface for loading links asynchronously, returning @@ -347,11 +347,11 @@ func (rm *RequestManager) PeerState(p peer.ID) peerstate.PeerState { // SendRequest sends a request to the message queue func (rm *RequestManager) SendRequest(p peer.ID, request gsmsg.GraphSyncRequest) { - sub := notifications.NewTopicDataSubscriber(&reqSubscriber{p, request, rm.networkErrorListeners}) - failNotifee := notifications.Notifee{Data: requestNetworkError, Subscriber: sub} - rm.peerHandler.AllocateAndBuildMessage(p, 0, func(builder *gsmsg.Builder) { + sub := &reqSubscriber{p, request, rm.networkErrorListeners} + rm.peerHandler.AllocateAndBuildMessage(p, 0, func(builder *messagequeue.Builder) { builder.AddRequest(request) - }, []notifications.Notifee{failNotifee}) + builder.SetSubscriber(request.ID(), sub) + }) } // Startup starts processing for the WantManager. @@ -378,7 +378,7 @@ type reqSubscriber struct { networkErrorListeners *listeners.NetworkErrorListeners } -func (r *reqSubscriber) OnNext(topic notifications.Topic, event notifications.Event) { +func (r *reqSubscriber) OnNext(_ notifications.Topic, event notifications.Event) { mqEvt, isMQEvt := event.(messagequeue.Event) if !isMQEvt || mqEvt.Name != messagequeue.Error { return @@ -389,7 +389,7 @@ func (r *reqSubscriber) OnNext(topic notifications.Topic, event notifications.Ev //r.re.terminateRequest() } -func (r reqSubscriber) OnClose(topic notifications.Topic) { +func (r reqSubscriber) OnClose(_ notifications.Topic) { } const requestNetworkError = "request_network_error" diff --git a/requestmanager/requestmanager_test.go b/requestmanager/requestmanager_test.go index c62d9702..a02e36eb 100644 --- a/requestmanager/requestmanager_test.go +++ b/requestmanager/requestmanager_test.go @@ -19,8 +19,8 @@ import ( "github.com/ipfs/go-graphsync/dedupkey" "github.com/ipfs/go-graphsync/listeners" gsmsg "github.com/ipfs/go-graphsync/message" + "github.com/ipfs/go-graphsync/messagequeue" "github.com/ipfs/go-graphsync/metadata" - "github.com/ipfs/go-graphsync/notifications" "github.com/ipfs/go-graphsync/requestmanager/executor" "github.com/ipfs/go-graphsync/requestmanager/hooks" "github.com/ipfs/go-graphsync/requestmanager/testloader" @@ -1014,8 +1014,8 @@ type fakePeerHandler struct { } func (fph *fakePeerHandler) AllocateAndBuildMessage(p peer.ID, blkSize uint64, - requestBuilder func(b *gsmsg.Builder), notifees []notifications.Notifee) { - builder := gsmsg.NewBuilder(gsmsg.Topic(0)) + requestBuilder func(b *messagequeue.Builder)) { + builder := messagequeue.NewBuilder(messagequeue.Topic(0)) requestBuilder(builder) message, err := builder.Build() if err != nil { diff --git a/responsemanager/client.go b/responsemanager/client.go index c3bf92a5..2280c74f 100644 --- a/responsemanager/client.go +++ b/responsemanager/client.go @@ -39,7 +39,6 @@ type inProgressResponseStatus struct { signals queryexecutor.ResponseSignals updates []gsmsg.GraphSyncRequest state graphsync.RequestState - subscriber *notifications.TopicDataSubscriber startTime time.Time responseStream responseassembler.ResponseStream } @@ -86,7 +85,7 @@ type NetworkErrorListeners interface { // ResponseAssembler is an interface that returns sender interfaces for peer responses. type ResponseAssembler interface { - NewStream(p peer.ID, requestID graphsync.RequestID) responseassembler.ResponseStream + NewStream(p peer.ID, requestID graphsync.RequestID, subscriber notifications.Subscriber) responseassembler.ResponseStream } type responseManagerMessage interface { diff --git a/responsemanager/queryexecutor/queryexecutor.go b/responsemanager/queryexecutor/queryexecutor.go index c1be1ff8..87a4708b 100644 --- a/responsemanager/queryexecutor/queryexecutor.go +++ b/responsemanager/queryexecutor/queryexecutor.go @@ -17,7 +17,6 @@ import ( "github.com/ipfs/go-graphsync" "github.com/ipfs/go-graphsync/ipldutil" gsmsg "github.com/ipfs/go-graphsync/message" - "github.com/ipfs/go-graphsync/notifications" "github.com/ipfs/go-graphsync/responsemanager/hooks" "github.com/ipfs/go-graphsync/responsemanager/responseassembler" ) @@ -39,7 +38,6 @@ const ErrFirstBlockLoad = errorString("Unable to load first block") // ResponseTask returns all information needed to execute a given response type ResponseTask struct { Empty bool - Subscriber *notifications.TopicDataSubscriber Ctx context.Context Span trace.Span Request gsmsg.GraphSyncRequest @@ -121,30 +119,28 @@ func (qe *QueryExecutor) executeQuery( // Execute the traversal operation, continue until we have reason to stop (error, pause, complete) err := qe.runTraversal(p, rt) + _, isPaused := err.(hooks.ErrPaused) + if isPaused { + return err + } + + if err == ErrNetworkError || ipldutil.IsContextCancelErr(err) { + rt.ResponseStream.ClearRequest() + return err + } + // Close out the response, either temporarily (pause) or permanently (cancel, fail, complete) return rt.ResponseStream.Transaction(func(rb responseassembler.ResponseBuilder) error { - var code graphsync.ResponseStatusCode - if err != nil { - _, isPaused := err.(hooks.ErrPaused) - if isPaused { - return err - } - if err == ErrNetworkError || ipldutil.IsContextCancelErr(err) { - rb.ClearRequest() - return err - } - if err == ErrFirstBlockLoad { - code = graphsync.RequestFailedContentNotFound - } else if err == ErrCancelledByCommand { - code = graphsync.RequestCancelled - } else { - code = graphsync.RequestFailedUnknown - } - rb.FinishWithError(code) - } else { - code = rb.FinishRequest() + switch err { + case nil: + rb.FinishRequest() + case ErrFirstBlockLoad: + rb.FinishWithError(graphsync.RequestFailedContentNotFound) + case ErrCancelledByCommand: + rb.FinishWithError(graphsync.RequestCancelled) + default: + rb.FinishWithError(graphsync.RequestFailedUnknown) } - rb.AddNotifee(notifications.Notifee{Data: code, Subscriber: rt.Subscriber}) return err }) } @@ -251,7 +247,6 @@ func (qe *QueryExecutor) sendResponse(p peer.ID, taskData ResponseTask, link ipl return err } blockData := rb.SendResponse(link, data) - rb.AddNotifee(notifications.Notifee{Data: blockData, Subscriber: taskData.Subscriber}) if blockData.BlockSize() > 0 { result := qe.blockHooks.ProcessBlockHooks(p, taskData.Request, blockData) for _, extension := range result.Extensions { @@ -287,5 +282,6 @@ type UpdateHooks interface { // ResponseStream is an interface that returns sender interfaces for peer responses. type ResponseStream interface { + ClearRequest() Transaction(transaction responseassembler.Transaction) error } diff --git a/responsemanager/queryexecutor/queryexecutor_test.go b/responsemanager/queryexecutor/queryexecutor_test.go index b7603a99..d3611d38 100644 --- a/responsemanager/queryexecutor/queryexecutor_test.go +++ b/responsemanager/queryexecutor/queryexecutor_test.go @@ -40,7 +40,6 @@ func TestEmptyTask(t *testing.T) { func TestOneBlockTask(t *testing.T) { td, qe := newTestData(t, 1, 1) defer td.cancel() - notifeeExpect(t, td, 1, td.responseCode) require.Equal(t, false, qe.ExecuteTask(td.ctx, td.peer, td.task)) require.Equal(t, 0, td.clearRequestCalls) } @@ -78,7 +77,6 @@ func TestSmallGraphTask(t *testing.T) { t.Run("full graph", func(t *testing.T) { td, qe := newTestData(t, 10, 10) defer td.cancel() - notifeeExpect(t, td, 10, td.responseCode) // AddNotifee called on all blocks require.Equal(t, false, qe.ExecuteTask(td.ctx, td.peer, td.task)) require.Equal(t, 0, td.clearRequestCalls) }) @@ -86,7 +84,6 @@ func TestSmallGraphTask(t *testing.T) { t.Run("paused by hook", func(t *testing.T) { td, qe := newTestData(t, 10, 7) defer td.cancel() - notifeeExpect(t, td, 7, nil) // AddNotifee called on first 7 blocks blockHookExpect(t, td, 6, func(hookActions graphsync.OutgoingBlockHookActions) { hookActions.PauseResponse() }, 6) @@ -100,7 +97,6 @@ func TestSmallGraphTask(t *testing.T) { t.Run("paused by signal", func(t *testing.T) { td, qe := newTestData(t, 10, 7) defer td.cancel() - notifeeExpect(t, td, 7, nil) // AddNotifee called on first 7 blocks blockHookExpect(t, td, 5, func(hookActions graphsync.OutgoingBlockHookActions) { select { case td.signals.PauseSignal <- struct{}{}: @@ -118,7 +114,6 @@ func TestSmallGraphTask(t *testing.T) { t.Run("partial cancelled by hook", func(t *testing.T) { td, qe := newTestData(t, 10, 7) defer td.cancel() - notifeeExpect(t, td, 7, nil) // AddNotifee called on first 7 blocks blockHookExpect(t, td, 6, func(hookActions graphsync.OutgoingBlockHookActions) { hookActions.TerminateWithError(ipldutil.ContextCancelError{}) }, 6) @@ -133,7 +128,6 @@ func TestSmallGraphTask(t *testing.T) { // unlike via blockhooks which is run after the block is sent td, qe := newTestData(t, 10, 7) defer td.cancel() - notifeeExpect(t, td, 6, graphsync.RequestCancelled) // AddNotifee called on first 6 blocks blockHookExpect(t, td, 5, func(hookActions graphsync.OutgoingBlockHookActions) { select { case td.signals.ErrSignal <- ErrCancelledByCommand: @@ -153,7 +147,6 @@ func TestSmallGraphTask(t *testing.T) { td, qe := newTestData(t, 10, 7) defer td.cancel() expectedErr := fmt.Errorf("derp") - notifeeExpect(t, td, 7, graphsync.RequestFailedUnknown) // AddNotifee called on first 7 blocks blockHookExpect(t, td, 6, func(hookActions graphsync.OutgoingBlockHookActions) { hookActions.TerminateWithError(expectedErr) }, 6) @@ -169,7 +162,6 @@ func TestSmallGraphTask(t *testing.T) { td, qe := newTestData(t, 10, 7) defer td.cancel() expectedErr := fmt.Errorf("derp") - notifeeExpect(t, td, 6, graphsync.RequestFailedUnknown) // AddNotifee called on first 6 blocks blockHookExpect(t, td, 5, func(hookActions graphsync.OutgoingBlockHookActions) { select { case td.signals.ErrSignal <- expectedErr: @@ -187,7 +179,6 @@ func TestSmallGraphTask(t *testing.T) { td, qe := newTestData(t, 10, 7) defer td.cancel() expectedErr := ErrNetworkError - notifeeExpect(t, td, 7, nil) // AddNotifee called on first 6 blocks blockHookExpect(t, td, 6, func(hookActions graphsync.OutgoingBlockHookActions) { hookActions.TerminateWithError(expectedErr) }, 6) @@ -203,7 +194,6 @@ func TestSmallGraphTask(t *testing.T) { td, qe := newTestData(t, 10, 7) defer td.cancel() expectedErr := ErrNetworkError - notifeeExpect(t, td, 6, nil) // AddNotifee called on first 6 blocks blockHookExpect(t, td, 5, func(hookActions graphsync.OutgoingBlockHookActions) { select { case td.signals.ErrSignal <- expectedErr: @@ -220,7 +210,6 @@ func TestSmallGraphTask(t *testing.T) { t.Run("first block wont load", func(t *testing.T) { td, qe := newTestData(t, 10, 7) defer td.cancel() - notifeeExpect(t, td, 0, graphsync.RequestFailedContentNotFound) // AddNotifee only called with error td.manager.responseTask.Traverser = &skipMeTraverser{} blockHookExpect(t, td, 0, func(hookActions graphsync.OutgoingBlockHookActions) {}, 0) transactionExpect(t, td, []int{0}, ErrFirstBlockLoad.Error()) @@ -230,23 +219,6 @@ func TestSmallGraphTask(t *testing.T) { }) } -func notifeeExpect(t *testing.T, td *testData, expectedCalls int, expectedFinalData notifications.TopicData) { - t.Helper() - notifeeCount := 1 - td.responseBuilder.notifeeCb = func(n notifications.Notifee) { - require.Same(t, td.subscriber, n.Subscriber) - if notifeeCount <= expectedCalls { - require.Same(t, td.expectedBlocks[notifeeCount-1], n.Data) - } else if notifeeCount == expectedCalls+1 { - // may not reach here in some cases - require.Equal(t, expectedFinalData, n.Data) - } else { - require.Fail(t, "too many notifee calls") - } - notifeeCount++ - } -} - func newRandomBlock(index int64) *blockData { digest := make([]byte, 32) _, err := rand.Read(digest) @@ -344,16 +316,16 @@ func newTestData(t *testing.T, blockCount int, expectedTraverse int) (*testData, t: t, finishRequest: td.responseCode, sendResponseCb: sendResponseCb, - clearRequestCb: func() { - td.clearRequestCalls++ - }, pauseCb: func() { require.Fail(t, "should not have called ResponseBuilder#PauseRequest()") }, } td.responseStream = &fauxResponseStream{ - t: t, + t: t, + clearRequestCb: func() { + td.clearRequestCalls++ + }, responseBuilder: td.responseBuilder, } @@ -378,7 +350,6 @@ func newTestData(t *testing.T, blockCount int, expectedTraverse int) (*testData, Loader: loader, Traverser: expectedTraverser, Signals: *td.signals, - Subscriber: td.subscriber, ResponseStream: td.responseStream, } td.responseStream.responseBuilder.pauseCb = func() { @@ -421,8 +392,14 @@ type fauxResponseStream struct { t *testing.T responseBuilder *fauxResponseBuilder transactionCb func(error) + clearRequestCb func() } +func (fra *fauxResponseStream) ClearRequest() { + if fra.clearRequestCb != nil { + fra.clearRequestCb() + } +} func (fra *fauxResponseStream) Transaction(transaction responseassembler.Transaction) error { var err error if fra.responseBuilder != nil { @@ -440,9 +417,7 @@ type fauxResponseBuilder struct { t *testing.T sendResponseCb func(ipld.Link, []byte) graphsync.BlockData finishRequest graphsync.ResponseStatusCode - notifeeCb func(n notifications.Notifee) pauseCb func() - clearRequestCb func() } func (rb fauxResponseBuilder) SendResponse(link ipld.Link, data []byte) graphsync.BlockData { @@ -452,12 +427,6 @@ func (rb fauxResponseBuilder) SendResponse(link ipld.Link, data []byte) graphsyn func (rb fauxResponseBuilder) SendExtensionData(ed graphsync.ExtensionData) { } -func (rb fauxResponseBuilder) ClearRequest() { - if rb.clearRequestCb != nil { - rb.clearRequestCb() - } -} - func (rb fauxResponseBuilder) FinishRequest() graphsync.ResponseStatusCode { return rb.finishRequest } @@ -471,12 +440,6 @@ func (rb fauxResponseBuilder) PauseRequest() { } } -func (rb fauxResponseBuilder) AddNotifee(n notifications.Notifee) { - if rb.notifeeCb != nil { - rb.notifeeCb(n) - } -} - var _ responseassembler.ResponseBuilder = &fauxResponseBuilder{} type blockData struct { diff --git a/responsemanager/querypreparer.go b/responsemanager/querypreparer.go index 7afb7a24..4b4b1391 100644 --- a/responsemanager/querypreparer.go +++ b/responsemanager/querypreparer.go @@ -16,7 +16,6 @@ import ( "github.com/ipfs/go-graphsync/donotsendfirstblocks" "github.com/ipfs/go-graphsync/ipldutil" gsmsg "github.com/ipfs/go-graphsync/message" - "github.com/ipfs/go-graphsync/notifications" "github.com/ipfs/go-graphsync/responsemanager/queryexecutor" "github.com/ipfs/go-graphsync/responsemanager/responseassembler" ) @@ -41,23 +40,18 @@ func (qe *queryPreparer) prepareQuery( p peer.ID, request gsmsg.GraphSyncRequest, responseStream responseassembler.ResponseStream, - signals queryexecutor.ResponseSignals, - sub *notifications.TopicDataSubscriber) (ipld.BlockReadOpener, ipldutil.Traverser, bool, error) { + signals queryexecutor.ResponseSignals) (ipld.BlockReadOpener, ipldutil.Traverser, bool, error) { result := qe.requestHooks.ProcessRequestHooks(p, request) var isPaused bool - failNotifee := notifications.Notifee{Data: graphsync.RequestFailedUnknown, Subscriber: sub} - rejectNotifee := notifications.Notifee{Data: graphsync.RequestRejected, Subscriber: sub} err := responseStream.Transaction(func(rb responseassembler.ResponseBuilder) error { for _, extension := range result.Extensions { rb.SendExtensionData(extension) } if result.Err != nil { rb.FinishWithError(graphsync.RequestFailedUnknown) - rb.AddNotifee(failNotifee) return result.Err } else if !result.IsValidated { rb.FinishWithError(graphsync.RequestRejected) - rb.AddNotifee(rejectNotifee) return errInvalidRequest } else if result.IsPaused { rb.PauseRequest() @@ -68,13 +62,13 @@ func (qe *queryPreparer) prepareQuery( if err != nil { return nil, nil, false, err } - if err := processDedupByKey(request, responseStream, failNotifee); err != nil { + if err := processDedupByKey(request, responseStream); err != nil { return nil, nil, false, err } - if err := processDoNoSendCids(request, responseStream, failNotifee); err != nil { + if err := processDoNoSendCids(request, responseStream); err != nil { return nil, nil, false, err } - if err := processDoNotSendFirstBlocks(request, responseStream, failNotifee); err != nil { + if err := processDoNotSendFirstBlocks(request, responseStream); err != nil { return nil, nil, false, err } rootLink := cidlink.Link{Cid: request.Root()} @@ -100,7 +94,7 @@ func (qe *queryPreparer) prepareQuery( return linkSystem.StorageReadOpener, traverser, isPaused, nil } -func processDedupByKey(request gsmsg.GraphSyncRequest, responseStream responseassembler.ResponseStream, failNotifee notifications.Notifee) error { +func processDedupByKey(request gsmsg.GraphSyncRequest, responseStream responseassembler.ResponseStream) error { dedupData, has := request.Extension(graphsync.ExtensionDeDupByKey) if !has { return nil @@ -109,7 +103,6 @@ func processDedupByKey(request gsmsg.GraphSyncRequest, responseStream responseas if err != nil { _ = responseStream.Transaction(func(rb responseassembler.ResponseBuilder) error { rb.FinishWithError(graphsync.RequestFailedUnknown) - rb.AddNotifee(failNotifee) return nil }) return err @@ -118,7 +111,7 @@ func processDedupByKey(request gsmsg.GraphSyncRequest, responseStream responseas return nil } -func processDoNoSendCids(request gsmsg.GraphSyncRequest, responseStream responseassembler.ResponseStream, failNotifee notifications.Notifee) error { +func processDoNoSendCids(request gsmsg.GraphSyncRequest, responseStream responseassembler.ResponseStream) error { doNotSendCidsData, has := request.Extension(graphsync.ExtensionDoNotSendCIDs) if !has { return nil @@ -127,7 +120,6 @@ func processDoNoSendCids(request gsmsg.GraphSyncRequest, responseStream response if err != nil { _ = responseStream.Transaction(func(rb responseassembler.ResponseBuilder) error { rb.FinishWithError(graphsync.RequestFailedUnknown) - rb.AddNotifee(failNotifee) return nil }) return err @@ -144,7 +136,7 @@ func processDoNoSendCids(request gsmsg.GraphSyncRequest, responseStream response return nil } -func processDoNotSendFirstBlocks(request gsmsg.GraphSyncRequest, responseStream responseassembler.ResponseStream, failNotifee notifications.Notifee) error { +func processDoNotSendFirstBlocks(request gsmsg.GraphSyncRequest, responseStream responseassembler.ResponseStream) error { doNotSendFirstBlocksData, has := request.Extension(graphsync.ExtensionsDoNotSendFirstBlocks) if !has { return nil @@ -153,7 +145,6 @@ func processDoNotSendFirstBlocks(request gsmsg.GraphSyncRequest, responseStream if err != nil { _ = responseStream.Transaction(func(rb responseassembler.ResponseBuilder) error { rb.FinishWithError(graphsync.RequestFailedUnknown) - rb.AddNotifee(failNotifee) return nil }) return err diff --git a/responsemanager/responseassembler/responseBuilder.go b/responsemanager/responseassembler/responseBuilder.go index 761491ad..c4ddefcb 100644 --- a/responsemanager/responseassembler/responseBuilder.go +++ b/responsemanager/responseassembler/responseBuilder.go @@ -7,21 +7,19 @@ import ( cidlink "github.com/ipld/go-ipld-prime/linking/cid" "github.com/ipfs/go-graphsync" - gsmsg "github.com/ipfs/go-graphsync/message" - "github.com/ipfs/go-graphsync/notifications" + "github.com/ipfs/go-graphsync/messagequeue" ) var log = logging.Logger("graphsync") type responseOperation interface { - build(builder *gsmsg.Builder) + build(builder *messagequeue.Builder) size() uint64 } type responseBuilder struct { requestID graphsync.RequestID operations []responseOperation - notifees []notifications.Notifee linkTracker *peerLinkTracker } @@ -49,14 +47,6 @@ func (rb *responseBuilder) PauseRequest() { rb.operations = append(rb.operations, statusOperation{rb.requestID, graphsync.RequestPaused}) } -func (rb *responseBuilder) ClearRequest() { - _ = rb.linkTracker.FinishTracking(rb.requestID) -} - -func (rb *responseBuilder) AddNotifee(notifee notifications.Notifee) { - rb.notifees = append(rb.notifees, notifee) -} - func (rb *responseBuilder) setupBlockOperation( link ipld.Link, data []byte) blockOperation { hasBlock := data != nil @@ -87,7 +77,7 @@ type statusOperation struct { status graphsync.ResponseStatusCode } -func (fo statusOperation) build(builder *gsmsg.Builder) { +func (fo statusOperation) build(builder *messagequeue.Builder) { builder.AddResponseCode(fo.requestID, fo.status) } @@ -100,7 +90,7 @@ type extensionOperation struct { extension graphsync.ExtensionData } -func (eo extensionOperation) build(builder *gsmsg.Builder) { +func (eo extensionOperation) build(builder *messagequeue.Builder) { builder.AddExtensionData(eo.requestID, eo.extension) } @@ -116,7 +106,7 @@ type blockOperation struct { index int64 } -func (bo blockOperation) build(builder *gsmsg.Builder) { +func (bo blockOperation) build(builder *messagequeue.Builder) { if bo.sendBlock { cidLink := bo.link.(cidlink.Link) block, err := blocks.NewBlockWithCid(bo.data, cidLink.Cid) @@ -126,6 +116,7 @@ func (bo blockOperation) build(builder *gsmsg.Builder) { builder.AddBlock(block) } builder.AddLink(bo.requestID, bo.link, bo.data != nil) + builder.AddBlockData(bo.requestID, bo.Block()) } func (bo blockOperation) size() uint64 { diff --git a/responsemanager/responseassembler/responseassembler.go b/responsemanager/responseassembler/responseassembler.go index 4b02e0c8..47cf03a3 100644 --- a/responsemanager/responseassembler/responseassembler.go +++ b/responsemanager/responseassembler/responseassembler.go @@ -15,7 +15,7 @@ import ( "github.com/libp2p/go-libp2p-core/peer" "github.com/ipfs/go-graphsync" - gsmsg "github.com/ipfs/go-graphsync/message" + "github.com/ipfs/go-graphsync/messagequeue" "github.com/ipfs/go-graphsync/notifications" "github.com/ipfs/go-graphsync/peermanager" ) @@ -35,9 +35,6 @@ type ResponseBuilder interface { // SendExtensionData adds extension data to the transaction. SendExtensionData(graphsync.ExtensionData) - // ClearRequest removes all tracking for this request. - ClearRequest() - // FinishRequest completes the response to a request. FinishRequest() graphsync.ResponseStatusCode @@ -46,15 +43,12 @@ type ResponseBuilder interface { // PauseRequest temporarily halts responding to the request PauseRequest() - - // AddNotifee adds a notifee to be notified about the response to request. - AddNotifee(notifications.Notifee) } // PeerMessageHandler is an interface that can queue a response for a given peer to go out over the network // If blkSize > 0, message building may block until enough memory has been freed from the queues to allocate the message. type PeerMessageHandler interface { - AllocateAndBuildMessage(p peer.ID, blkSize uint64, buildResponseFn func(*gsmsg.Builder), notifees []notifications.Notifee) + AllocateAndBuildMessage(p peer.ID, blkSize uint64, buildResponseFn func(*messagequeue.Builder)) } // ResponseAssembler manages assembling responses to go out over the network @@ -74,12 +68,13 @@ func New(ctx context.Context, peerHandler PeerMessageHandler) *ResponseAssembler } } -func (ra *ResponseAssembler) NewStream(p peer.ID, requestID graphsync.RequestID) ResponseStream { +func (ra *ResponseAssembler) NewStream(p peer.ID, requestID graphsync.RequestID, subscriber notifications.Subscriber) ResponseStream { return &responseStream{ requestID: requestID, p: p, messageSenders: ra.peerHandler, linkTrackers: ra.PeerManager, + subscriber: subscriber, } } @@ -90,6 +85,7 @@ type responseStream struct { closedLk sync.RWMutex messageSenders PeerMessageHandler linkTrackers *peermanager.PeerManager + subscriber notifications.Subscriber } func (r *responseStream) Close() error { @@ -110,6 +106,8 @@ type ResponseStream interface { DedupKey(key string) IgnoreBlocks(links []ipld.Link) SkipFirstBlocks(skipFirstBlocks int64) + // ClearRequest removes all tracking for this request. + ClearRequest() } // DedupKey indicates that outgoing blocks should be deduplicated in a seperate bucket (only with requests that share @@ -128,17 +126,22 @@ func (rs *responseStream) SkipFirstBlocks(skipFirstBlocks int64) { rs.linkTrackers.GetProcess(rs.p).(*peerLinkTracker).SkipFirstBlocks(rs.requestID, skipFirstBlocks) } +// ClearRequest removes all tracking for this request. +func (rs *responseStream) ClearRequest() { + _ = rs.linkTrackers.GetProcess(rs.p).(*peerLinkTracker).FinishTracking(rs.requestID) +} + func (rs *responseStream) Transaction(transaction Transaction) error { rb := &responseBuilder{ requestID: rs.requestID, linkTracker: rs.linkTrackers.GetProcess(rs.p).(*peerLinkTracker), } err := transaction(rb) - rs.execute(rb.operations, rb.notifees) + rs.execute(rb.operations) return err } -func (rs *responseStream) execute(operations []responseOperation, notifees []notifications.Notifee) { +func (rs *responseStream) execute(operations []responseOperation) { size := uint64(0) for _, op := range operations { size += op.size() @@ -146,13 +149,14 @@ func (rs *responseStream) execute(operations []responseOperation, notifees []not if rs.isClosed() { return } - rs.messageSenders.AllocateAndBuildMessage(rs.p, size, func(builder *gsmsg.Builder) { + rs.messageSenders.AllocateAndBuildMessage(rs.p, size, func(builder *messagequeue.Builder) { if rs.isClosed() { return } for _, op := range operations { op.build(builder) } - builder.AddResponseStream(rs.requestID, rs) - }, notifees) + builder.SetResponseStream(rs.requestID, rs) + builder.SetSubscriber(rs.requestID, rs.subscriber) + }) } diff --git a/responsemanager/responseassembler/responseassembler_test.go b/responsemanager/responseassembler/responseassembler_test.go index bc88e15a..edce6a70 100644 --- a/responsemanager/responseassembler/responseassembler_test.go +++ b/responsemanager/responseassembler/responseassembler_test.go @@ -16,6 +16,7 @@ import ( "github.com/ipfs/go-graphsync" gsmsg "github.com/ipfs/go-graphsync/message" + "github.com/ipfs/go-graphsync/messagequeue" "github.com/ipfs/go-graphsync/notifications" "github.com/ipfs/go-graphsync/testutil" ) @@ -28,9 +29,6 @@ func TestResponseAssemblerSendsResponses(t *testing.T) { requestID1 := graphsync.RequestID(rand.Int31()) requestID2 := graphsync.RequestID(rand.Int31()) requestID3 := graphsync.RequestID(rand.Int31()) - sendResponseNotifee1, _ := testutil.NewTestNotifee(requestID1, 10) - sendResponseNotifee2, _ := testutil.NewTestNotifee(requestID2, 10) - sendResponseNotifee3, _ := testutil.NewTestNotifee(requestID3, 10) blks := testutil.GenerateBlocksOfSize(5, 100) links := make([]ipld.Link, 0, len(blks)) @@ -42,30 +40,31 @@ func TestResponseAssemblerSendsResponses(t *testing.T) { var bd1, bd2 graphsync.BlockData - stream1 := responseAssembler.NewStream(p, requestID1) - stream2 := responseAssembler.NewStream(p, requestID2) - stream3 := responseAssembler.NewStream(p, requestID3) + sub1 := testutil.NewTestSubscriber(10) + stream1 := responseAssembler.NewStream(p, requestID1, sub1) + sub2 := testutil.NewTestSubscriber(10) + stream2 := responseAssembler.NewStream(p, requestID2, sub2) + sub3 := testutil.NewTestSubscriber(10) + stream3 := responseAssembler.NewStream(p, requestID3, sub3) // send block 0 for request 1 require.NoError(t, stream1.Transaction(func(b ResponseBuilder) error { - b.AddNotifee(sendResponseNotifee1) bd1 = b.SendResponse(links[0], blks[0].RawData()) return nil })) assertSentOnWire(t, bd1, blks[0]) fph.AssertBlocks(blks[0]) fph.AssertResponses(expectedResponses{requestID1: graphsync.PartialResponse}) - fph.AssertNotifees(sendResponseNotifee1) + fph.AssertSubscriber(requestID1, sub1) fph.AssertResponseStream(requestID1, stream1) // send block 0 for request 2 (duplicate block should not be sent) require.NoError(t, stream2.Transaction(func(b ResponseBuilder) error { - b.AddNotifee(sendResponseNotifee2) bd1 = b.SendResponse(links[0], blks[0].RawData()) return nil })) assertSentNotOnWire(t, bd1, blks[0]) fph.AssertResponses(expectedResponses{requestID2: graphsync.PartialResponse}) - fph.AssertNotifees(sendResponseNotifee2) + fph.AssertSubscriber(requestID2, sub2) fph.AssertResponseStream(requestID2, stream2) // send more to request 1 and finish request @@ -83,6 +82,7 @@ func TestResponseAssemblerSendsResponses(t *testing.T) { fph.AssertResponses(expectedResponses{ requestID1: graphsync.RequestCompletedPartial, }) + fph.AssertSubscriber(requestID1, sub1) fph.AssertResponseStream(requestID1, stream1) // send more to request 2 @@ -95,6 +95,7 @@ func TestResponseAssemblerSendsResponses(t *testing.T) { fph.AssertResponses(expectedResponses{ requestID2: graphsync.RequestCompletedFull, }) + fph.AssertSubscriber(requestID2, sub2) fph.AssertResponseStream(requestID2, stream2) // send to request 3 @@ -106,11 +107,11 @@ func TestResponseAssemblerSendsResponses(t *testing.T) { fph.AssertResponses(expectedResponses{ requestID3: graphsync.PartialResponse, }) + fph.AssertSubscriber(requestID3, sub3) fph.AssertResponseStream(requestID3, stream3) // send 2 more to request 3 require.NoError(t, stream3.Transaction(func(b ResponseBuilder) error { - b.AddNotifee(sendResponseNotifee3) bd1 = b.SendResponse(links[0], blks[0].RawData()) bd1 = b.SendResponse(links[4], blks[4].RawData()) return nil @@ -118,6 +119,7 @@ func TestResponseAssemblerSendsResponses(t *testing.T) { fph.AssertBlocks(blks[0]) fph.AssertResponses(expectedResponses{requestID3: graphsync.PartialResponse}) + fph.AssertSubscriber(requestID3, sub3) fph.AssertResponseStream(requestID3, stream3) } @@ -135,13 +137,15 @@ func TestResponseAssemblerCloseStream(t *testing.T) { fph := newFakePeerHandler(ctx, t) responseAssembler := New(ctx, fph) - stream1 := responseAssembler.NewStream(p, requestID1) + sub1 := testutil.NewTestSubscriber(10) + stream1 := responseAssembler.NewStream(p, requestID1, sub1) require.NoError(t, stream1.Transaction(func(b ResponseBuilder) error { b.SendResponse(links[0], blks[0].RawData()) return nil })) fph.AssertBlocks(blks[0]) fph.AssertResponses(expectedResponses{requestID1: graphsync.PartialResponse}) + fph.AssertSubscriber(requestID1, sub1) fph.AssertResponseStream(requestID1, stream1) // close the response stream @@ -169,7 +173,8 @@ func TestResponseAssemblerSendsExtensionData(t *testing.T) { fph := newFakePeerHandler(ctx, t) responseAssembler := New(ctx, fph) - stream1 := responseAssembler.NewStream(p, requestID1) + sub1 := testutil.NewTestSubscriber(10) + stream1 := responseAssembler.NewStream(p, requestID1, sub1) require.NoError(t, stream1.Transaction(func(b ResponseBuilder) error { b.SendResponse(links[0], blks[0].RawData()) return nil @@ -214,29 +219,30 @@ func TestResponseAssemblerSendsResponsesInTransaction(t *testing.T) { } fph := newFakePeerHandler(ctx, t) responseAssembler := New(ctx, fph) - notifee, _ := testutil.NewTestNotifee("transaction", 10) - stream1 := responseAssembler.NewStream(p, requestID1) + sub1 := testutil.NewTestSubscriber(10) + stream1 := responseAssembler.NewStream(p, requestID1, sub1) + var bd1, bd2, bd3 graphsync.BlockData err := stream1.Transaction(func(b ResponseBuilder) error { - bd := b.SendResponse(links[0], blks[0].RawData()) - assertSentOnWire(t, bd, blks[0]) + bd1 = b.SendResponse(links[0], blks[0].RawData()) + assertSentOnWire(t, bd1, blks[0]) fph.RefuteHasMessage() fph.RefuteBlocks() fph.RefuteResponses() - bd = b.SendResponse(links[1], blks[1].RawData()) - assertSentOnWire(t, bd, blks[1]) - bd = b.SendResponse(links[2], nil) - assertNotSent(t, bd, blks[2]) + bd2 = b.SendResponse(links[1], blks[1].RawData()) + assertSentOnWire(t, bd2, blks[1]) + bd3 = b.SendResponse(links[2], nil) + assertNotSent(t, bd3, blks[2]) b.FinishRequest() - b.AddNotifee(notifee) fph.RefuteHasMessage() return nil }) require.NoError(t, err) - - fph.AssertNotifees(notifee) + fph.AssertBlockData(requestID1, bd1) + fph.AssertBlockData(requestID1, bd2) + fph.AssertBlockData(requestID1, bd3) } func TestResponseAssemblerIgnoreBlocks(t *testing.T) { @@ -253,8 +259,10 @@ func TestResponseAssemblerIgnoreBlocks(t *testing.T) { } fph := newFakePeerHandler(ctx, t) responseAssembler := New(ctx, fph) - stream1 := responseAssembler.NewStream(p, requestID1) - stream2 := responseAssembler.NewStream(p, requestID2) + sub1 := testutil.NewTestSubscriber(10) + stream1 := responseAssembler.NewStream(p, requestID1, sub1) + sub2 := testutil.NewTestSubscriber(10) + stream2 := responseAssembler.NewStream(p, requestID2, sub2) stream1.IgnoreBlocks(links) @@ -269,7 +277,7 @@ func TestResponseAssemblerIgnoreBlocks(t *testing.T) { assertSentNotOnWire(t, bd1, blks[0]) fph.RefuteBlocks() fph.AssertResponses(expectedResponses{requestID1: graphsync.PartialResponse}) - + fph.AssertBlockData(requestID1, bd1) err = stream2.Transaction(func(b ResponseBuilder) error { bd1 = b.SendResponse(links[0], blks[0].RawData()) return nil @@ -278,6 +286,7 @@ func TestResponseAssemblerIgnoreBlocks(t *testing.T) { fph.AssertResponses(expectedResponses{ requestID2: graphsync.PartialResponse, }) + fph.AssertBlockData(requestID2, bd1) err = stream1.Transaction(func(b ResponseBuilder) error { bd2 = b.SendResponse(links[1], blks[1].RawData()) @@ -295,9 +304,12 @@ func TestResponseAssemblerIgnoreBlocks(t *testing.T) { fph.AssertResponses(expectedResponses{ requestID1: graphsync.RequestCompletedFull, }) + fph.AssertBlockData(requestID1, bd2) + fph.AssertBlockData(requestID1, bd3) + var bd4 graphsync.BlockData err = stream2.Transaction(func(b ResponseBuilder) error { - b.SendResponse(links[3], blks[3].RawData()) + bd4 = b.SendResponse(links[3], blks[3].RawData()) b.FinishRequest() return nil }) @@ -305,7 +317,7 @@ func TestResponseAssemblerIgnoreBlocks(t *testing.T) { fph.AssertBlocks(blks[3]) fph.AssertResponses(expectedResponses{requestID2: graphsync.RequestCompletedFull}) - + fph.AssertBlockData(requestID2, bd4) } func TestResponseAssemblerSkipFirstBlocks(t *testing.T) { @@ -323,8 +335,10 @@ func TestResponseAssemblerSkipFirstBlocks(t *testing.T) { fph := newFakePeerHandler(ctx, t) responseAssembler := New(ctx, fph) - stream1 := responseAssembler.NewStream(p, requestID1) - stream2 := responseAssembler.NewStream(p, requestID2) + sub1 := testutil.NewTestSubscriber(10) + stream1 := responseAssembler.NewStream(p, requestID1, sub1) + sub2 := testutil.NewTestSubscriber(10) + stream2 := responseAssembler.NewStream(p, requestID2, sub2) stream1.SkipFirstBlocks(3) @@ -339,6 +353,7 @@ func TestResponseAssemblerSkipFirstBlocks(t *testing.T) { assertSentNotOnWire(t, bd1, blks[0]) fph.RefuteBlocks() fph.AssertResponses(expectedResponses{requestID1: graphsync.PartialResponse}) + fph.AssertBlockData(requestID1, bd1) err = stream2.Transaction(func(b ResponseBuilder) error { bd1 = b.SendResponse(links[0], blks[0].RawData()) @@ -348,6 +363,7 @@ func TestResponseAssemblerSkipFirstBlocks(t *testing.T) { fph.AssertResponses(expectedResponses{ requestID2: graphsync.PartialResponse, }) + fph.AssertBlockData(requestID2, bd1) err = stream1.Transaction(func(b ResponseBuilder) error { bd2 = b.SendResponse(links[1], blks[1].RawData()) @@ -364,6 +380,9 @@ func TestResponseAssemblerSkipFirstBlocks(t *testing.T) { fph.AssertResponses(expectedResponses{ requestID1: graphsync.PartialResponse, }) + fph.AssertBlockData(requestID1, bd2) + fph.AssertBlockData(requestID1, bd3) + err = stream1.Transaction(func(b ResponseBuilder) error { bd4 = b.SendResponse(links[3], blks[3].RawData()) bd5 = b.SendResponse(links[4], blks[4].RawData()) @@ -377,6 +396,8 @@ func TestResponseAssemblerSkipFirstBlocks(t *testing.T) { fph.AssertBlocks(blks[3], blks[4]) fph.AssertResponses(expectedResponses{requestID1: graphsync.RequestCompletedFull}) + fph.AssertBlockData(requestID1, bd4) + fph.AssertBlockData(requestID1, bd5) err = stream2.Transaction(func(b ResponseBuilder) error { b.SendResponse(links[3], blks[3].RawData()) @@ -405,9 +426,12 @@ func TestResponseAssemblerDupKeys(t *testing.T) { } fph := newFakePeerHandler(ctx, t) responseAssembler := New(ctx, fph) - stream1 := responseAssembler.NewStream(p, requestID1) - stream2 := responseAssembler.NewStream(p, requestID2) - stream3 := responseAssembler.NewStream(p, requestID3) + sub1 := testutil.NewTestSubscriber(10) + stream1 := responseAssembler.NewStream(p, requestID1, sub1) + sub2 := testutil.NewTestSubscriber(10) + stream2 := responseAssembler.NewStream(p, requestID2, sub2) + sub3 := testutil.NewTestSubscriber(10) + stream3 := responseAssembler.NewStream(p, requestID3, sub3) stream1.DedupKey("applesauce") stream3.DedupKey("applesauce") @@ -423,6 +447,7 @@ func TestResponseAssemblerDupKeys(t *testing.T) { fph.AssertBlocks(blks[0]) fph.AssertResponses(expectedResponses{requestID1: graphsync.PartialResponse}) + fph.AssertBlockData(requestID1, bd1) err = stream2.Transaction(func(b ResponseBuilder) error { bd1 = b.SendResponse(links[0], blks[0].RawData()) @@ -430,6 +455,7 @@ func TestResponseAssemblerDupKeys(t *testing.T) { }) require.NoError(t, err) assertSentOnWire(t, bd1, blks[0]) + fph.AssertBlockData(requestID2, bd1) err = stream1.Transaction(func(b ResponseBuilder) error { bd1 = b.SendResponse(links[1], blks[1].RawData()) @@ -442,6 +468,8 @@ func TestResponseAssemblerDupKeys(t *testing.T) { fph.AssertBlocks(blks[1]) fph.AssertResponses(expectedResponses{requestID1: graphsync.PartialResponse}) + fph.AssertBlockData(requestID1, bd1) + fph.AssertBlockData(requestID1, bd2) err = stream2.Transaction(func(b ResponseBuilder) error { b.SendResponse(links[3], blks[3].RawData()) @@ -507,7 +535,8 @@ type fakePeerHandler struct { lastResponseStreams map[graphsync.RequestID]io.Closer lastBlocks []blocks.Block lastResponses []gsmsg.GraphSyncResponse - lastNotifiees []notifications.Notifee + lastSubscribers map[graphsync.RequestID]notifications.Subscriber + lastBlockData map[graphsync.RequestID][]graphsync.BlockData sent chan struct{} } @@ -515,6 +544,8 @@ func newFakePeerHandler(ctx context.Context, t *testing.T) *fakePeerHandler { t.Helper() return &fakePeerHandler{ lastResponseStreams: map[graphsync.RequestID]io.Closer{}, + lastSubscribers: map[graphsync.RequestID]notifications.Subscriber{}, + lastBlockData: map[graphsync.RequestID][]graphsync.BlockData{}, ctx: ctx, t: t, } @@ -571,37 +602,49 @@ func (fph *fakePeerHandler) AssertExtensions(extensionSets [][]graphsync.Extensi } } -func (fph *fakePeerHandler) AssertNotifees(notifees ...notifications.Notifee) { - require.Len(fph.t, fph.lastNotifiees, len(notifees)) - for i, notifee := range notifees { - require.Equal(fph.t, notifee, fph.lastNotifiees[i]) - } +func (fph *fakePeerHandler) AssertSubscriber(requestID graphsync.RequestID, expected notifications.Subscriber) { + actual, ok := fph.lastSubscribers[requestID] + require.True(fph.t, ok) + require.Equal(fph.t, expected, actual) +} + +func (fph *fakePeerHandler) AssertBlockData(requestID graphsync.RequestID, expected graphsync.BlockData) { + actual, ok := fph.lastBlockData[requestID] + require.True(fph.t, ok) + require.Contains(fph.t, actual, expected) } func (fph *fakePeerHandler) RefuteResponses() { require.Empty(fph.t, fph.lastResponses) } -func (fph *fakePeerHandler) AllocateAndBuildMessage(p peer.ID, blkSize uint64, buildMessageFn func(*gsmsg.Builder), notifees []notifications.Notifee) { - builder := gsmsg.NewBuilder(gsmsg.Topic(0)) +func (fph *fakePeerHandler) AllocateAndBuildMessage(p peer.ID, blkSize uint64, buildMessageFn func(*messagequeue.Builder)) { + builder := messagequeue.NewBuilder(messagequeue.Topic(0)) buildMessageFn(builder) msg, err := builder.Build() require.NoError(fph.t, err) - fph.sendResponse(p, msg.Responses(), msg.Blocks(), builder.ResponseStreams(), notifees...) + fph.sendResponse(p, msg.Responses(), msg.Blocks(), builder.ResponseStreams(), builder.Subscribers(), builder.BlockData()) } -func (fph *fakePeerHandler) sendResponse(p peer.ID, responses []gsmsg.GraphSyncResponse, blks []blocks.Block, responseStreams map[graphsync.RequestID]io.Closer, notifees ...notifications.Notifee) { +func (fph *fakePeerHandler) sendResponse(p peer.ID, + responses []gsmsg.GraphSyncResponse, + blks []blocks.Block, + responseStreams map[graphsync.RequestID]io.Closer, + subscribers map[graphsync.RequestID]notifications.Subscriber, + blockData map[graphsync.RequestID][]graphsync.BlockData) { fph.lastResponses = responses fph.lastBlocks = blks - fph.lastNotifiees = notifees fph.lastResponseStreams = responseStreams + fph.lastSubscribers = subscribers + fph.lastBlockData = blockData } func (fph *fakePeerHandler) Clear() { fph.lastResponses = nil - fph.lastNotifiees = nil + fph.lastSubscribers = map[graphsync.RequestID]notifications.Subscriber{} + fph.lastBlockData = map[graphsync.RequestID][]graphsync.BlockData{} fph.lastResponseStreams = map[graphsync.RequestID]io.Closer{} fph.lastBlocks = nil } diff --git a/responsemanager/responsemanager_test.go b/responsemanager/responsemanager_test.go index 0af5c319..d9d5fc46 100644 --- a/responsemanager/responsemanager_test.go +++ b/responsemanager/responsemanager_test.go @@ -834,7 +834,7 @@ func TestNetworkErrors(t *testing.T) { td.assertPausedRequest() err := errors.New("something went wrong") td.notifyBlockSendsNetworkError(err) - td.assertNetworkErrors(err, blockCount) + td.assertNetworkErrors(err, 1) td.assertRequestCleared() err = responseManager.UnpauseResponse(td.p, td.requestID, td.extensionResponse) require.Error(t, err) @@ -850,12 +850,16 @@ type fakeResponseAssembler struct { clearedRequests chan clearedRequest ignoredLinks chan []ipld.Link skippedFirstBlocks chan int64 - notifeePublisher *testutil.MockPublisher - dedupKeys chan string - missingBlock bool + + completedNotifications map[graphsync.RequestID]graphsync.ResponseStatusCode + blkNotifications map[graphsync.RequestID][]graphsync.BlockData + notifeePublisher *testutil.MockPublisher + dedupKeys chan string + missingBlock bool } -func (fra *fakeResponseAssembler) NewStream(p peer.ID, requestID graphsync.RequestID) responseassembler.ResponseStream { +func (fra *fakeResponseAssembler) NewStream(p peer.ID, requestID graphsync.RequestID, subscriber notifications.Subscriber) responseassembler.ResponseStream { + fra.notifeePublisher.AddSubscriber(subscriber) return &fakeResponseStream{fra, requestID} } @@ -867,8 +871,7 @@ type fakeResponseStream struct { func (frs *fakeResponseStream) Transaction(transaction responseassembler.Transaction) error { frs.fra.transactionLk.Lock() defer frs.fra.transactionLk.Unlock() - frb := &fakeResponseBuilder{frs.requestID, frs.fra, - frs.fra.notifeePublisher} + frb := &fakeResponseBuilder{frs.requestID, frs.fra} return transaction(frb) } @@ -883,6 +886,10 @@ func (frs *fakeResponseStream) DedupKey(key string) { frs.fra.dedupKeys <- key } +func (frs *fakeResponseStream) ClearRequest() { + frs.fra.clearRequest(frs.requestID) +} + type sentResponse struct { requestID graphsync.RequestID link ipld.Link @@ -936,7 +943,9 @@ func (fra *fakeResponseAssembler) sendResponse( if data == nil { fra.missingBlock = true } - return fakeBlkData{link, uint64(len(data))} + blkData := fakeBlkData{link, uint64(len(data))} + fra.blkNotifications[requestID] = append(fra.blkNotifications[requestID], blkData) + return blkData } func (fra *fakeResponseAssembler) sendExtensionData( @@ -952,13 +961,17 @@ func (fra *fakeResponseAssembler) finishRequest(requestID graphsync.RequestID) g code = graphsync.RequestCompletedPartial } fra.missingBlock = false - fra.lastCompletedRequest <- completedRequest{requestID, code} + cr := completedRequest{requestID, code} + fra.lastCompletedRequest <- cr + fra.completedNotifications[requestID] = code return code } func (fra *fakeResponseAssembler) finishWithError(requestID graphsync.RequestID, status graphsync.ResponseStatusCode) { fra.missingBlock = false - fra.lastCompletedRequest <- completedRequest{requestID, status} + cr := completedRequest{requestID, status} + fra.lastCompletedRequest <- cr + fra.completedNotifications[requestID] = status } func (fra *fakeResponseAssembler) pauseRequest(requestID graphsync.RequestID) { @@ -971,9 +984,8 @@ func (fra *fakeResponseAssembler) clearRequest(requestID graphsync.RequestID) { } type fakeResponseBuilder struct { - requestID graphsync.RequestID - fra *fakeResponseAssembler - notifeePublisher *testutil.MockPublisher + requestID graphsync.RequestID + fra *fakeResponseAssembler } func (frb *fakeResponseBuilder) SendResponse(link ipld.Link, data []byte) graphsync.BlockData { @@ -996,14 +1008,6 @@ func (frb *fakeResponseBuilder) PauseRequest() { frb.fra.pauseRequest(frb.requestID) } -func (frb *fakeResponseBuilder) ClearRequest() { - frb.fra.clearRequest(frb.requestID) -} - -func (frb *fakeResponseBuilder) AddNotifee(notifee notifications.Notifee) { - frb.notifeePublisher.AddNotifees([]notifications.Notifee{notifee}) -} - type testData struct { ctx context.Context t *testing.T @@ -1017,6 +1021,8 @@ type testData struct { sentExtensions chan sentExtension pausedRequests chan pausedRequest clearedRequests chan clearedRequest + completedNotifications map[graphsync.RequestID]graphsync.ResponseStatusCode + blkNotifications map[graphsync.RequestID][]graphsync.BlockData ignoredLinks chan []ipld.Link skippedFirstBlocks chan int64 dedupKeys chan string @@ -1076,17 +1082,21 @@ func newTestData(t *testing.T) testData { td.networkErrorChan = make(chan error, td.blockChainLength*2) td.notifeePublisher = testutil.NewMockPublisher() td.transactionLk = &sync.Mutex{} + td.completedNotifications = make(map[graphsync.RequestID]graphsync.ResponseStatusCode) + td.blkNotifications = make(map[graphsync.RequestID][]graphsync.BlockData) td.responseAssembler = &fakeResponseAssembler{ - transactionLk: td.transactionLk, - lastCompletedRequest: td.completedRequestChan, - sentResponses: td.sentResponses, - sentExtensions: td.sentExtensions, - pausedRequests: td.pausedRequests, - clearedRequests: td.clearedRequests, - ignoredLinks: td.ignoredLinks, - skippedFirstBlocks: td.skippedFirstBlocks, - dedupKeys: td.dedupKeys, - notifeePublisher: td.notifeePublisher, + transactionLk: td.transactionLk, + lastCompletedRequest: td.completedRequestChan, + sentResponses: td.sentResponses, + sentExtensions: td.sentExtensions, + pausedRequests: td.pausedRequests, + clearedRequests: td.clearedRequests, + ignoredLinks: td.ignoredLinks, + skippedFirstBlocks: td.skippedFirstBlocks, + dedupKeys: td.dedupKeys, + notifeePublisher: td.notifeePublisher, + blkNotifications: td.blkNotifications, + completedNotifications: td.completedNotifications, } td.extensionData = testutil.RandomBytes(100) @@ -1274,37 +1284,41 @@ func (td *testData) assertSkippedFirstBlocks(expectedSkipCount int64) { func (td *testData) notifyStatusMessagesSent() { td.transactionLk.Lock() - td.notifeePublisher.PublishMatchingEvents(func(data notifications.TopicData) bool { - _, isSn := data.(graphsync.ResponseStatusCode) - return isSn - }, []notifications.Event{messagequeue.Event{Name: messagequeue.Sent}}) + td.notifeePublisher.PublishEvents(notifications.Topic(rand.Int31), []notifications.Event{ + messagequeue.Event{Name: messagequeue.Sent, Metadata: messagequeue.Metadata{ResponseCodes: td.completedNotifications}}, + }) + td.completedNotifications = make(map[graphsync.RequestID]graphsync.ResponseStatusCode) + td.responseAssembler.completedNotifications = td.completedNotifications td.transactionLk.Unlock() } func (td *testData) notifyBlockSendsSent() { td.transactionLk.Lock() - td.notifeePublisher.PublishMatchingEvents(func(data notifications.TopicData) bool { - _, isBsn := data.(graphsync.BlockData) - return isBsn - }, []notifications.Event{messagequeue.Event{Name: messagequeue.Sent}}) + td.notifeePublisher.PublishEvents(notifications.Topic(rand.Int31), []notifications.Event{ + messagequeue.Event{Name: messagequeue.Sent, Metadata: messagequeue.Metadata{BlockData: td.blkNotifications}}, + }) + td.blkNotifications = make(map[graphsync.RequestID][]graphsync.BlockData) + td.responseAssembler.blkNotifications = td.blkNotifications td.transactionLk.Unlock() } func (td *testData) notifyStatusMessagesNetworkError(err error) { td.transactionLk.Lock() - td.notifeePublisher.PublishMatchingEvents(func(data notifications.TopicData) bool { - _, isSn := data.(graphsync.ResponseStatusCode) - return isSn - }, []notifications.Event{messagequeue.Event{Name: messagequeue.Error, Err: err}}) + td.notifeePublisher.PublishEvents(notifications.Topic(rand.Int31), []notifications.Event{ + messagequeue.Event{Name: messagequeue.Error, Err: err, Metadata: messagequeue.Metadata{ResponseCodes: td.completedNotifications}}, + }) + td.completedNotifications = make(map[graphsync.RequestID]graphsync.ResponseStatusCode) + td.responseAssembler.completedNotifications = td.completedNotifications td.transactionLk.Unlock() } func (td *testData) notifyBlockSendsNetworkError(err error) { td.transactionLk.Lock() - td.notifeePublisher.PublishMatchingEvents(func(data notifications.TopicData) bool { - _, isBsn := data.(graphsync.BlockData) - return isBsn - }, []notifications.Event{messagequeue.Event{Name: messagequeue.Error, Err: err}}) + td.notifeePublisher.PublishEvents(notifications.Topic(rand.Int31), []notifications.Event{ + messagequeue.Event{Name: messagequeue.Error, Err: err, Metadata: messagequeue.Metadata{BlockData: td.blkNotifications}}, + }) + td.blkNotifications = make(map[graphsync.RequestID][]graphsync.BlockData) + td.responseAssembler.blkNotifications = td.blkNotifications td.transactionLk.Unlock() } diff --git a/responsemanager/server.go b/responsemanager/server.go index 5460a798..465c11e1 100644 --- a/responsemanager/server.go +++ b/responsemanager/server.go @@ -17,7 +17,6 @@ import ( "github.com/ipfs/go-graphsync" "github.com/ipfs/go-graphsync/ipldutil" gsmsg "github.com/ipfs/go-graphsync/message" - "github.com/ipfs/go-graphsync/notifications" "github.com/ipfs/go-graphsync/peerstate" "github.com/ipfs/go-graphsync/responsemanager/hooks" "github.com/ipfs/go-graphsync/responsemanager/queryexecutor" @@ -85,7 +84,6 @@ func (rm *ResponseManager) processUpdate(key responseKey, update gsmsg.GraphSync } if result.Err != nil { rb.FinishWithError(graphsync.RequestFailedUnknown) - rb.AddNotifee(notifications.Notifee{Data: graphsync.RequestFailedUnknown, Subscriber: response.subscriber}) } return nil }) @@ -143,22 +141,22 @@ func (rm *ResponseManager) abortRequest(p peer.ID, requestID graphsync.RequestID response.span.SetStatus(codes.Error, err.Error()) if response.state != graphsync.Running { - _ = response.responseStream.Transaction(func(rb responseassembler.ResponseBuilder) error { - if ipldutil.IsContextCancelErr(err) { - rm.cancelledListeners.NotifyCancelledListeners(p, response.request) - rb.ClearRequest() - rm.terminateRequest(key) - } else if err == queryexecutor.ErrNetworkError { - rb.ClearRequest() - rm.terminateRequest(key) - } else { - rb.FinishWithError(graphsync.RequestCancelled) - rb.AddNotifee(notifications.Notifee{Data: graphsync.RequestCancelled, Subscriber: response.subscriber}) - response.state = graphsync.CompletingSend - } + if ipldutil.IsContextCancelErr(err) { + response.responseStream.ClearRequest() + rm.terminateRequest(key) + rm.cancelledListeners.NotifyCancelledListeners(p, response.request) + return nil + } + if err == queryexecutor.ErrNetworkError { + response.responseStream.ClearRequest() + rm.terminateRequest(key) + return nil + } + response.state = graphsync.CompletingSend + return response.responseStream.Transaction(func(rb responseassembler.ResponseBuilder) error { + rb.FinishWithError(graphsync.RequestCancelled) return nil }) - return nil } select { case response.signals.ErrSignal <- err: @@ -187,7 +185,7 @@ func (rm *ResponseManager) processRequests(p peer.ID, requests []gsmsg.GraphSync attribute.StringSlice("extensions", request.ExtensionNames()), )) ctx, cancelFn := context.WithCancel(ctx) - sub := notifications.NewTopicDataSubscriber(&subscriber{ + sub := &subscriber{ p: key.p, request: request, requestCloser: rm, @@ -195,7 +193,7 @@ func (rm *ResponseManager) processRequests(p peer.ID, requests []gsmsg.GraphSync completedListeners: rm.completedListeners, networkErrorListeners: rm.networkErrorListeners, connManager: rm.connManager, - }) + } log.Infow("graphsync request initiated", "request id", request.ID(), "peer", p, "root", request.Root()) ipr, ok := rm.inProgressResponses[key] if ok && ipr.state == graphsync.Running { @@ -204,11 +202,10 @@ func (rm *ResponseManager) processRequests(p peer.ID, requests []gsmsg.GraphSync rm.inProgressResponses[key] = &inProgressResponseStatus{ - ctx: ctx, - span: responseSpan, - cancelFn: cancelFn, - subscriber: sub, - request: request, + ctx: ctx, + span: responseSpan, + cancelFn: cancelFn, + request: request, signals: queryexecutor.ResponseSignals{ PauseSignal: make(chan struct{}, 1), UpdateSignal: make(chan struct{}, 1), @@ -216,7 +213,7 @@ func (rm *ResponseManager) processRequests(p peer.ID, requests []gsmsg.GraphSync }, state: graphsync.Queued, startTime: time.Now(), - responseStream: rm.responseAssembler.NewStream(key.p, key.requestID), + responseStream: rm.responseAssembler.NewStream(key.p, key.requestID, sub), } // TODO: Use a better work estimation metric. @@ -232,7 +229,7 @@ func (rm *ResponseManager) taskDataForKey(key responseKey) queryexecutor.Respons log.Infow("graphsync response processing begins", "request id", key.requestID, "peer", key.p, "total time", time.Since(response.startTime)) if response.loader == nil || response.traverser == nil { - loader, traverser, isPaused, err := (&queryPreparer{rm.requestHooks, rm.linkSystem, rm.maxLinksPerRequest}).prepareQuery(response.ctx, key.p, response.request, response.responseStream, response.signals, response.subscriber) + loader, traverser, isPaused, err := (&queryPreparer{rm.requestHooks, rm.linkSystem, rm.maxLinksPerRequest}).prepareQuery(response.ctx, key.p, response.request, response.responseStream, response.signals) if err != nil { response.state = graphsync.CompletingSend response.span.RecordError(err) @@ -251,7 +248,6 @@ func (rm *ResponseManager) taskDataForKey(key responseKey) queryexecutor.Respons Ctx: response.ctx, Span: response.span, Empty: false, - Subscriber: response.subscriber, Request: response.request, Loader: response.loader, Traverser: response.traverser, diff --git a/responsemanager/subscriber.go b/responsemanager/subscriber.go index 70e682a0..8e3992e7 100644 --- a/responsemanager/subscriber.go +++ b/responsemanager/subscriber.go @@ -26,36 +26,32 @@ type subscriber struct { connManager network.ConnManager } -func (s *subscriber) OnNext(topic notifications.Topic, event notifications.Event) { +func (s *subscriber) OnNext(_ notifications.Topic, event notifications.Event) { responseEvent, ok := event.(messagequeue.Event) if !ok { return } - blockData, isBlockData := topic.(graphsync.BlockData) - if isBlockData { - switch responseEvent.Name { - case messagequeue.Error: - s.networkErrorListeners.NotifyNetworkErrorListeners(s.p, s.request, responseEvent.Err) - s.requestCloser.CloseWithNetworkError(s.p, s.request.ID()) - case messagequeue.Sent: + switch responseEvent.Name { + case messagequeue.Error: + s.requestCloser.CloseWithNetworkError(s.p, s.request.ID()) + responseCode := responseEvent.Metadata.ResponseCodes[s.request.ID()] + if responseCode.IsTerminal() { + s.requestCloser.TerminateRequest(s.p, s.request.ID()) + } + s.networkErrorListeners.NotifyNetworkErrorListeners(s.p, s.request, responseEvent.Err) + case messagequeue.Sent: + blockDatas := responseEvent.Metadata.BlockData[s.request.ID()] + for _, blockData := range blockDatas { s.blockSentListeners.NotifyBlockSentListeners(s.p, s.request, blockData) } - return - } - status, isStatus := topic.(graphsync.ResponseStatusCode) - if isStatus { - switch responseEvent.Name { - case messagequeue.Error: - s.requestCloser.CloseWithNetworkError(s.p, s.request.ID()) - s.requestCloser.TerminateRequest(s.p, s.request.ID()) - s.networkErrorListeners.NotifyNetworkErrorListeners(s.p, s.request, responseEvent.Err) - case messagequeue.Sent: + responseCode := responseEvent.Metadata.ResponseCodes[s.request.ID()] + if responseCode.IsTerminal() { s.requestCloser.TerminateRequest(s.p, s.request.ID()) - s.completedListeners.NotifyCompletedListeners(s.p, s.request, status) + s.completedListeners.NotifyCompletedListeners(s.p, s.request, responseCode) } } } -func (s *subscriber) OnClose(topic notifications.Topic) { +func (s *subscriber) OnClose(_ notifications.Topic) { } diff --git a/testutil/testnotifications.go b/testutil/testnotifications.go index f6709dfa..51051141 100644 --- a/testutil/testnotifications.go +++ b/testutil/testnotifications.go @@ -43,6 +43,14 @@ func (ts *TestSubscriber) ExpectEvents(ctx context.Context, t *testing.T, events require.Equal(t, expectedEvent, event) } } +func (ts *TestSubscriber) ExpectEventsAllTopics(ctx context.Context, t *testing.T, events []notifications.Event) { + t.Helper() + for _, expectedEvent := range events { + var event DispatchedEvent + AssertReceive(ctx, t, ts.receivedEvents, &event, "should receive another event") + require.Equal(t, expectedEvent, event.Event) + } +} func (ts *TestSubscriber) NoEventsReceived(t *testing.T) { t.Helper() @@ -62,6 +70,13 @@ func (ts *TestSubscriber) ExpectClosesAnyOrder(ctx context.Context, t *testing.T require.Equal(t, expectedTopics, receivedTopics) } +func (ts *TestSubscriber) ExpectNCloses(ctx context.Context, t *testing.T, n int) { + t.Helper() + for i := 0; i < n; i++ { + AssertDoesReceive(ctx, t, ts.closed, "should receive another event") + } +} + func (ts *TestSubscriber) ExpectCloses(ctx context.Context, t *testing.T, topics []notifications.Topic) { t.Helper() for _, expectedTopic := range topics { @@ -102,47 +117,26 @@ func NewTestNotifee(data notifications.TopicData, bufferSize int) (notifications } type MockPublisher struct { - notifeesLk sync.Mutex - notifees []notifications.Notifee -} - -func (mp *MockPublisher) AddNotifees(notifees []notifications.Notifee) { - mp.notifeesLk.Lock() - mp.notifees = append(mp.notifees, notifees...) - mp.notifeesLk.Unlock() -} - -func (mp *MockPublisher) PublishMatchingEvents(shouldPublish func(notifications.TopicData) bool, events []notifications.Event) { - mp.notifeesLk.Lock() - var newNotifees []notifications.Notifee - for _, notifee := range mp.notifees { - if shouldPublish(notifee.Data) { - for _, ev := range events { - notifee.Subscriber.Subscriber.OnNext(notifee.Data, ev) - } - notifee.Subscriber.Subscriber.OnClose(notifee.Data) - } else { - newNotifees = append(newNotifees, notifee) - } - } - mp.notifees = newNotifees - mp.notifeesLk.Unlock() + subscribersLk sync.Mutex + subscribers []notifications.Subscriber } -func (mp *MockPublisher) PublishEvents(events []notifications.Event) { - mp.PublishMatchingEvents(func(notifications.TopicData) bool { return true }, events) +func (mp *MockPublisher) AddSubscriber(subscriber notifications.Subscriber) { + mp.subscribersLk.Lock() + mp.subscribers = append(mp.subscribers, subscriber) + mp.subscribersLk.Unlock() } -func (mp *MockPublisher) PublishEventsOnTopicData(data []notifications.TopicData, events []notifications.Event) { - shouldPublish := func(topic notifications.TopicData) bool { - for _, testTopicData := range data { - if topic == testTopicData { - return true - } +func (mp *MockPublisher) PublishEvents(topic notifications.Topic, events []notifications.Event) { + mp.subscribersLk.Lock() + for _, subscriber := range mp.subscribers { + + for _, ev := range events { + subscriber.OnNext(topic, ev) } - return false + subscriber.OnClose(topic) } - mp.PublishMatchingEvents(shouldPublish, events) + mp.subscribersLk.Unlock() } func NewMockPublisher() *MockPublisher { From 150b0dd7aadc08d4c5177f36d7ec4e79f9502dd9 Mon Sep 17 00:00:00 2001 From: hannahhoward Date: Wed, 15 Dec 2021 18:00:16 -0800 Subject: [PATCH 07/12] feat(notification): remove the topic data subscriber --- notifications/data_subscriber.go | 54 ------------------- notifications/notifications_test.go | 9 ---- notifications/types.go | 17 ------ .../queryexecutor/queryexecutor_test.go | 3 -- testutil/testnotifications.go | 30 ----------- 5 files changed, 113 deletions(-) delete mode 100644 notifications/data_subscriber.go diff --git a/notifications/data_subscriber.go b/notifications/data_subscriber.go deleted file mode 100644 index f8643bb1..00000000 --- a/notifications/data_subscriber.go +++ /dev/null @@ -1,54 +0,0 @@ -package notifications - -import ( - "sync" -) - -type TopicDataSubscriber struct { - idMapLk sync.RWMutex - data map[Topic][]TopicData - Subscriber -} - -// NewTopicDataSubscriber produces a subscriber that will transform -// events and topics before passing them on to the given subscriber -func NewTopicDataSubscriber(sub Subscriber) *TopicDataSubscriber { - return &TopicDataSubscriber{ - Subscriber: sub, - data: make(map[Topic][]TopicData), - } -} - -func (m *TopicDataSubscriber) AddTopicData(id Topic, data TopicData) { - m.idMapLk.Lock() - m.data[id] = append(m.data[id], data) - m.idMapLk.Unlock() -} - -func (m *TopicDataSubscriber) getData(id Topic) []TopicData { - m.idMapLk.RLock() - defer m.idMapLk.RUnlock() - - data, ok := m.data[id] - if !ok { - return []TopicData{} - } - newData := make([]TopicData, len(data)) - copy(newData, data) - return newData -} - -func (m *TopicDataSubscriber) OnNext(topic Topic, ev Event) { - for _, data := range m.getData(topic) { - m.Subscriber.OnNext(data, ev) - } -} - -func (m *TopicDataSubscriber) OnClose(topic Topic) { - for _, data := range m.getData(topic) { - m.Subscriber.OnClose(data) - } - m.idMapLk.Lock() - delete(m.data, topic) - m.idMapLk.Unlock() -} diff --git a/notifications/notifications_test.go b/notifications/notifications_test.go index 9c2838d7..73af3099 100644 --- a/notifications/notifications_test.go +++ b/notifications/notifications_test.go @@ -12,15 +12,6 @@ import ( func TestSubscribeWithData(t *testing.T) { ctx := context.Background() testCases := map[string]func(ctx context.Context, t *testing.T, ps notifications.Publisher){ - "SubscribeWithData": func(ctx context.Context, t *testing.T, ps notifications.Publisher) { - destTopic := "t2" - notifee, verifier := testutil.NewTestNotifee(destTopic, 1) - notifications.SubscribeWithData(ps, "t1", notifee) - ps.Publish("t1", "hi") - ps.Shutdown() - verifier.ExpectEvents(ctx, t, []notifications.Event{"hi"}) - verifier.ExpectClose(ctx, t) - }, "Add subscriptions": func(ctx context.Context, t *testing.T, ps notifications.Publisher) { sub1 := testutil.NewTestSubscriber(3) sub2 := testutil.NewTestSubscriber(3) diff --git a/notifications/types.go b/notifications/types.go index a6332631..716500fe 100644 --- a/notifications/types.go +++ b/notifications/types.go @@ -29,20 +29,3 @@ type Publisher interface { Startup() Subscribable } - -// EventTransform if a fucntion transforms one kind of event to another -type EventTransform func(Event) Event - -// Notifee is a topic data subscriber plus a set of data you want to add to any topics subscribed to -// (used to call SubscribeWithData to inject data when events for a given topic emit) -type Notifee struct { - Data TopicData - Subscriber *TopicDataSubscriber -} - -// SubscribeWithData subscribes to the given subscriber on the given topic, and adds the notifies -// custom data into the list of data injected into callbacks when events occur on that topic -func SubscribeWithData(p Subscribable, topic Topic, notifee Notifee) { - notifee.Subscriber.AddTopicData(topic, notifee.Data) - p.Subscribe(topic, notifee.Subscriber) -} diff --git a/responsemanager/queryexecutor/queryexecutor_test.go b/responsemanager/queryexecutor/queryexecutor_test.go index d3611d38..aefc736c 100644 --- a/responsemanager/queryexecutor/queryexecutor_test.go +++ b/responsemanager/queryexecutor/queryexecutor_test.go @@ -24,7 +24,6 @@ import ( "github.com/ipfs/go-graphsync" "github.com/ipfs/go-graphsync/ipldutil" gsmsg "github.com/ipfs/go-graphsync/message" - "github.com/ipfs/go-graphsync/notifications" "github.com/ipfs/go-graphsync/responsemanager/hooks" "github.com/ipfs/go-graphsync/responsemanager/responseassembler" "github.com/ipfs/go-graphsync/testutil" @@ -261,7 +260,6 @@ type testData struct { expectedBlocks []*blockData responseCode graphsync.ResponseStatusCode peer peer.ID - subscriber *notifications.TopicDataSubscriber } func newTestData(t *testing.T, blockCount int, expectedTraverse int) (*testData, *QueryExecutor) { @@ -283,7 +281,6 @@ func newTestData(t *testing.T, blockCount int, expectedTraverse int) (*testData, td.extensionName = graphsync.ExtensionName("AppleSauce/McGee") td.responseCode = graphsync.ResponseStatusCode(101) td.peer = testutil.GeneratePeers(1)[0] - td.subscriber = ¬ifications.TopicDataSubscriber{} td.extension = graphsync.ExtensionData{ Name: td.extensionName, diff --git a/testutil/testnotifications.go b/testutil/testnotifications.go index 51051141..4fdab9e2 100644 --- a/testutil/testnotifications.go +++ b/testutil/testnotifications.go @@ -86,36 +86,6 @@ func (ts *TestSubscriber) ExpectCloses(ctx context.Context, t *testing.T, topics } } -type NotifeeVerifier struct { - expectedTopic notifications.Topic - subscriber *TestSubscriber -} - -func (nv *NotifeeVerifier) ExpectEvents(ctx context.Context, t *testing.T, events []notifications.Event) { - t.Helper() - dispatchedEvents := make([]DispatchedEvent, 0, len(events)) - for _, ev := range events { - dispatchedEvents = append(dispatchedEvents, DispatchedEvent{nv.expectedTopic, ev}) - } - nv.subscriber.ExpectEvents(ctx, t, dispatchedEvents) -} - -func (nv *NotifeeVerifier) ExpectClose(ctx context.Context, t *testing.T) { - t.Helper() - nv.subscriber.ExpectCloses(ctx, t, []notifications.Topic{nv.expectedTopic}) -} - -func NewTestNotifee(data notifications.TopicData, bufferSize int) (notifications.Notifee, *NotifeeVerifier) { - subscriber := NewTestSubscriber(bufferSize) - return notifications.Notifee{ - Data: data, - Subscriber: notifications.NewTopicDataSubscriber(subscriber), - }, &NotifeeVerifier{ - expectedTopic: data, - subscriber: subscriber, - } -} - type MockPublisher struct { subscribersLk sync.Mutex subscribers []notifications.Subscriber From 4c95a0db54fdb40231e1a5fdece6b48e335c6ad4 Mon Sep 17 00:00:00 2001 From: hannahhoward Date: Wed, 15 Dec 2021 18:19:04 -0800 Subject: [PATCH 08/12] fix(responsemanager): make CloseWithNetworkError synchronous --- responsemanager/client.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/responsemanager/client.go b/responsemanager/client.go index 2280c74f..fc31595a 100644 --- a/responsemanager/client.go +++ b/responsemanager/client.go @@ -225,7 +225,12 @@ func (rm *ResponseManager) FinishTask(task *peertask.Task, err error) { // CloseWithNetworkError closes a request due to a network error func (rm *ResponseManager) CloseWithNetworkError(p peer.ID, requestID graphsync.RequestID) { - rm.send(&errorRequestMessage{p, requestID, queryexecutor.ErrNetworkError, make(chan error, 1)}, nil) + done := make(chan error, 1) + rm.send(&errorRequestMessage{p, requestID, queryexecutor.ErrNetworkError, done}, nil) + select { + case <-rm.ctx.Done(): + case <-done: + } } // TerminateRequest indicates a request has finished sending data and should no longer be tracked From 11202940e57e7536e636a8ab887d6679549966e1 Mon Sep 17 00:00:00 2001 From: hannahhoward Date: Wed, 15 Dec 2021 18:29:28 -0800 Subject: [PATCH 09/12] style(graphsync): fix fmt errors --- messagequeue/messagequeue_test.go | 2 +- requestmanager/client.go | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/messagequeue/messagequeue_test.go b/messagequeue/messagequeue_test.go index 7358e0f4..ef7e69e5 100644 --- a/messagequeue/messagequeue_test.go +++ b/messagequeue/messagequeue_test.go @@ -165,7 +165,7 @@ func TestProcessingNotification(t *testing.T) { responseID: status, }, BlockData: map[graphsync.RequestID][]graphsync.BlockData{ - responseID: []graphsync.BlockData{blkData}, + responseID: {blkData}, }, } subscriber.ExpectEventsAllTopics(ctx, t, []notifications.Event{ diff --git a/requestmanager/client.go b/requestmanager/client.go index b0996c24..2f0cdb6a 100644 --- a/requestmanager/client.go +++ b/requestmanager/client.go @@ -383,13 +383,8 @@ func (r *reqSubscriber) OnNext(_ notifications.Topic, event notifications.Event) if !isMQEvt || mqEvt.Name != messagequeue.Error { return } - r.networkErrorListeners.NotifyNetworkErrorListeners(r.p, r.request, mqEvt.Err) - //r.re.networkError <- mqEvt.Err - //r.re.terminateRequest() } func (r reqSubscriber) OnClose(_ notifications.Topic) { } - -const requestNetworkError = "request_network_error" From 6b4fb24be6e2b9d7ed98e8d1ae8ae3a3ee9ea1cf Mon Sep 17 00:00:00 2001 From: Hannah Howard Date: Wed, 15 Dec 2021 18:30:06 -0800 Subject: [PATCH 10/12] Update impl/graphsync_test.go per @rvagg Co-authored-by: Rod Vagg --- impl/graphsync_test.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/impl/graphsync_test.go b/impl/graphsync_test.go index b85b4553..9ef6b20e 100644 --- a/impl/graphsync_test.go +++ b/impl/graphsync_test.go @@ -979,12 +979,13 @@ func TestNetworkDisconnect(t *testing.T) { tracing := collectTracing(t) traceStrings := tracing.TracesToStrings() - require.Contains(t, traceStrings, "response(0)->executeTask(0)") - require.Contains(t, traceStrings, "response(0)->abortRequest(0)") - require.NotContains(t, traceStrings, "response(0)->abortRequest(1)") - // may contain multiple abortRequest traces as the network error can bubble up >1 times - // but these will only record if the request is still executing - require.Contains(t, traceStrings, "request(0)->newRequest(0)") + require.ElementsMatch(t, []string{ + "response(0)->executeTask(0)", + "response(0)->abortRequest(1)", + "request(0)->newRequest(0)", + "request(0)->executeTask(0)", + "request(0)->terminateRequest(0)", + }, tracing.TracesToStrings()) require.Contains(t, traceStrings, "request(0)->executeTask(0)") require.Contains(t, traceStrings, "request(0)->terminateRequest(0)") // has ContextCancelError exception recorded in the right place From d194251af48a0124d2c555658dc50fd65aeca804 Mon Sep 17 00:00:00 2001 From: hannahhoward Date: Wed, 15 Dec 2021 18:45:47 -0800 Subject: [PATCH 11/12] fix(impl): fix span sequence --- impl/graphsync_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/impl/graphsync_test.go b/impl/graphsync_test.go index 9ef6b20e..dba60504 100644 --- a/impl/graphsync_test.go +++ b/impl/graphsync_test.go @@ -978,16 +978,14 @@ func TestNetworkDisconnect(t *testing.T) { drain(responder) tracing := collectTracing(t) - traceStrings := tracing.TracesToStrings() require.ElementsMatch(t, []string{ "response(0)->executeTask(0)", - "response(0)->abortRequest(1)", + "response(0)->abortRequest(0)", + "response(0)->executeTask(1)", "request(0)->newRequest(0)", "request(0)->executeTask(0)", "request(0)->terminateRequest(0)", }, tracing.TracesToStrings()) - require.Contains(t, traceStrings, "request(0)->executeTask(0)") - require.Contains(t, traceStrings, "request(0)->terminateRequest(0)") // has ContextCancelError exception recorded in the right place tracing.SingleExceptionEvent(t, "request(0)->executeTask(0)", "ContextCancelError", ipldutil.ContextCancelError{}.Error(), false) } From 483d91d8ae043234dded1cd632071554e6e218a7 Mon Sep 17 00:00:00 2001 From: hannahhoward Date: Thu, 16 Dec 2021 17:24:35 -0800 Subject: [PATCH 12/12] feat(responseassembler): move up closed check --- responsemanager/responseassembler/responseassembler.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/responsemanager/responseassembler/responseassembler.go b/responsemanager/responseassembler/responseassembler.go index 47cf03a3..ff2ca505 100644 --- a/responsemanager/responseassembler/responseassembler.go +++ b/responsemanager/responseassembler/responseassembler.go @@ -142,13 +142,13 @@ func (rs *responseStream) Transaction(transaction Transaction) error { } func (rs *responseStream) execute(operations []responseOperation) { + if rs.isClosed() { + return + } size := uint64(0) for _, op := range operations { size += op.size() } - if rs.isClosed() { - return - } rs.messageSenders.AllocateAndBuildMessage(rs.p, size, func(builder *messagequeue.Builder) { if rs.isClosed() { return