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

Cancelable ReadDirContext #565

Merged
merged 2 commits into from
Nov 20, 2023
Merged

Cancelable ReadDirContext #565

merged 2 commits into from
Nov 20, 2023

Conversation

ungerik
Copy link
Contributor

@ungerik ungerik commented Nov 10, 2023

Added method Client.ReadDirContext to be able to cancel long-running dir listings.

The passed context can cancel the operation returning all entries listed up to the cancellation.

client.go Outdated
var done = false
for !done {
if err = ctx.Err(); err != nil {
return entries, err
}
id := c.nextID()
typ, data, err1 := c.sendPacket(nil, &sshFxpReaddirPacket{
Copy link
Collaborator

Choose a reason for hiding this comment

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

This creates internally a channel, that is used to waiting on for a response from the server. https://github.com/pkg/sftp/blob/master/conn.go#L131

Instead we would want to use dispatchRequest and then we can:

select {
case <-ctx.Done():
  return entries, err
case result := <-ch:
  typ, data, err1 = s.typ, s.data, s.err
}

Otherwise, we’re going to be stuck until the server responds with at least one sshFxpName packet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I have added the context to opendir and sendPacket

@puellanivis
Copy link
Collaborator

puellanivis commented Nov 13, 2023

Hm… probably not the change I would have made… but probably a change for the better… 🤔

P.S.: That is to say, I would have limited the scope of the change rather than plumb a context everywhere. But then, the context should probably be there anyways, so. 🤷‍♀️

@ungerik
Copy link
Contributor Author

ungerik commented Nov 15, 2023

It has become a reflex to add ctx everywhere. So far it never turned out that there were too many ctx in any code base, if there were problems then because of not enough ctx ;-)

@ungerik
Copy link
Contributor Author

ungerik commented Nov 15, 2023

But I can rewrite the PR if you want...

@puellanivis
Copy link
Collaborator

No, there is no need to rewrite, I don’t think the plumbed context will cause any performance issues.

@puellanivis puellanivis merged commit 5bdc2b0 into pkg:master Nov 20, 2023
4 checks passed
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