Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track actual network operations in a response #102

Merged
merged 6 commits into from
Oct 13, 2020

Conversation

hannahhoward
Copy link
Collaborator

Goals

Provide a way on the responding side to know how much data has gone over the wire and to know when actual network errors occur

Implementation

Essentially, or goal here is a pub-sub system to escalate asyncronous notifications back through the chain of callers to provide information to the response manager about what actually happened with data it attempted to send over the wire

  • build a simple pub/sub topic system, with with a key feature -- the ability to remap topics -- I subscribe on one topic but receive notifications on another
  • in the message queue, publish notifications when things happen with a message: it's queued, it's successfully sent, or an error occurs
  • the the peer response sender use the queue event to insure one messages is processed at a time, and republish response sender events based of the message event
  • in the response manager map semantically meaningful topics on to response sender events -- so that when we receive a sent or an error, we have all the information we need to react to it
  • convert listeners to only taking the requestID -- it strikes me that someone listening should already have picked up the request ID from an incoming request hook. Also, this makes it much easier to pack relevant information into the topic
  • listen for semantically meaningful events and now pass on to two new hooks -- OnBlockSentListener -- listens for when the block actually goes over the wire, and NetworkErrorListener -- listens for when a message going over the wire encounters an error
  • refactor the peerresponsesender and response manager tests significantly -- these were both extremely verbose

For Discussion

It's possible the notification system is unneccesarily complicated. It seems like a useful tool for the future though.

build an architecture via which we can pass on notifications
pass notifications on from peer response sender based on message queue notifications
Pass on network errors through the response manager, pass block sent errors as well
@rvagg
Copy link
Member

rvagg commented Oct 13, 2020

Wow, that's quite a bit of code! I wish I could help with a meaningful review, I've done a skim but I don't think I have anything helpful to add. So 🤷 if it seems to be working then you're in the best position to make a call on it. This doesn't do anything at the protocol level, does it? Alternative implementations don't need to care about anything you've added here do they?

@hannahhoward
Copy link
Collaborator Author

@rvagg I appreciate the review... it's unfortunate that I don't have a better guide to this code... once I've got distance form filecoin I'm going to work on making this code base simpler.

In the meantime,

Nope, no protocol changes. Alternative implementations would only care in as much as they want to provide the same functionality to their consumers... (i.e. effective notifications about what's going on with your request) -- which is to say, completely optional.

@hannahhoward hannahhoward merged commit dd6fa45 into master Oct 13, 2020
@aschmahmann aschmahmann mentioned this pull request Feb 18, 2021
73 tasks
@mvdan mvdan deleted the feat/track-actual-sends branch December 15, 2021 14:16
marten-seemann pushed a commit that referenced this pull request Mar 2, 2023
* feat(graphsync): make compatible across protocol

while we had setup message comptability over libp2p we had not handled if the other peer only sends
message1_0 over graphsync. Support this by always sending both extensions when possible and
defaulting to 1_1 when present

* fix(deps): update to tagged go-graphsync
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants