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

Unpause, Pause, and Cancel methods should return an error channel (with max 1 value sent) rather than an error #250

Open
hannahhoward opened this issue Oct 21, 2021 · 0 comments
Labels
effort/hours Estimated to take one or several hours exp/intermediate Prior experience is likely helpful need/analysis Needs further analysis before proceeding P3 Low: Not priority right now

Comments

@hannahhoward
Copy link
Collaborator

hannahhoward commented Oct 21, 2021

What

Convert the following methods on the GraphsyncExchange interface:

type GraphsyncExchange interface {

//...

	// UnpauseRequest unpauses a request that was paused in a block hook based request ID
	// Can also send extensions with unpause
	UnpauseRequest(RequestID, ...ExtensionData) error

	// PauseRequest pauses an in progress request (may take 1 or more blocks to process)
	PauseRequest(RequestID) error

	// UnpauseResponse unpauses a response that was paused in a block hook based on peer ID and request ID
	// Can also send extensions with unpause
	UnpauseResponse(peer.ID, RequestID, ...ExtensionData) error

	// PauseResponse pauses an in progress response (may take 1 or more blocks to process)
	PauseResponse(peer.ID, RequestID) error

	// CancelResponse cancels an in progress response
	CancelResponse(peer.ID, RequestID) error

	// CancelRequest cancels an in progress request
	CancelRequest(context.Context, RequestID) error

//...
}

to

type GraphsyncExchange interface {

//...

	// UnpauseRequest unpauses a request that was paused in a block hook based request ID
	// Can also send extensions with unpause
	UnpauseRequest(RequestID, ...ExtensionData) <-chan error

	// PauseRequest pauses an in progress request (may take 1 or more blocks to process)
	PauseRequest(RequestID)  <-chan error

	// UnpauseResponse unpauses a response that was paused in a block hook based on peer ID and request ID
	// Can also send extensions with unpause
	UnpauseResponse(peer.ID, RequestID, ...ExtensionData)  <-chan error

	// PauseResponse pauses an in progress response (may take 1 or more blocks to process)
	PauseResponse(peer.ID, RequestID)  <-chan error

	// CancelResponse cancels an in progress response
	CancelResponse(peer.ID, RequestID)  <-chan error

	// CancelRequest cancels an in progress request
	CancelRequest(context.Context, RequestID)  <-chan error

//...
}

Why

The above methods delegate to the RequestManager and ResponseManager, and if you look at the code for their implementation, they all perform a channel wait implicitly. This makes them significantly more blocking than they need to be, and perhaps we want to let calling code decide when to wait for the results.

In particular we may want to unlock mutexs between the call and the wait.

Flagging as needs discussion as its a breaking change

@hannahhoward hannahhoward added need/triage Needs initial labeling and prioritization effort/hours Estimated to take one or several hours exp/intermediate Prior experience is likely helpful need/analysis Needs further analysis before proceeding P1 High: Likely tackled by core team if no one steps up and removed need/triage Needs initial labeling and prioritization labels Oct 21, 2021
@rvagg rvagg self-assigned this Oct 22, 2021
@hannahhoward hannahhoward added P2 Medium: Good to have, but can wait until someone steps up and removed P1 High: Likely tackled by core team if no one steps up labels Oct 26, 2021
@hannahhoward hannahhoward added P3 Low: Not priority right now and removed P2 Medium: Good to have, but can wait until someone steps up labels Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/hours Estimated to take one or several hours exp/intermediate Prior experience is likely helpful need/analysis Needs further analysis before proceeding P3 Low: Not priority right now
Projects
None yet
Development

No branches or pull requests

2 participants