Skip to content

Cancelable ReadDirContext#565

Merged
puellanivis merged 2 commits into
pkg:masterfrom
ungerik:master
Nov 20, 2023
Merged

Cancelable ReadDirContext#565
puellanivis merged 2 commits into
pkg:masterfrom
ungerik:master

Conversation

@ungerik
Copy link
Copy Markdown
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.

Comment thread client.go Outdated
return entries, err
}
id := c.nextID()
typ, data, err1 := c.sendPacket(nil, &sshFxpReaddirPacket{
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

ungerik commented Nov 15, 2023

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

@puellanivis
Copy link
Copy Markdown
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
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