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

Prevent concurrent restarts for same channel #195

Merged
merged 1 commit into from
Apr 15, 2021

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Apr 14, 2021

This PR makes sure that when the data transfer is restarted for a pull request (retrieval), it cannot be interrupted by a second concurrent restart request for the same data transfer channel.

@dirkmc dirkmc added the x/ignite Issues and PRs being tracked by Team Ignite at Protocol Labs label Apr 14, 2021
@dirkmc dirkmc self-assigned this Apr 14, 2021
@dirkmc dirkmc requested a review from nonsense April 14, 2021 15:17
Comment on lines +25 to +30
// When restarting a data transfer, we cancel the existing graphsync request
// before opening a new one.
// These constants define the minimum and maximum time to wait for the request
// to be cancelled.
const minGSCancelWait = 100 * time.Millisecond
const maxGSCancelWait = time.Second
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, I am just not sure how we decide that these values are reasonable...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's true, ideally there would be a way to ask graphsync to cancel a request and wait until all events have been emitted before returning

@dirkmc dirkmc merged commit dbc57eb into master Apr 15, 2021
@dirkmc dirkmc deleted the fix/lock-chan-on-restart branch April 15, 2021 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x/ignite Issues and PRs being tracked by Team Ignite at Protocol Labs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants