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

Add local file deletion on `torrent.destroy` #1102

Open
wants to merge 4 commits into
base: master
from

Conversation

@Rowern
Copy link

Rowern commented Apr 13, 2017

When we call torrent.destroy and we are not running in a browser, we use
rimraf to delete the folder containing the torrent files.

This is a work in progress because I need feedback on a good way to implement it.
For instance, client.destroy calls torrent.destroy on all its torrents, this might be problematic as destroying the client looks like the legitimate way of stopping every torrent transactions but keeping your progress.

When we call `torrent.destroy` and we are not running in a browser, we use
`rimraf` to delete the folder containing the torrent files.
@feross

This comment has been minimized.

Copy link
Member

feross commented Apr 13, 2017

Thanks for the PR, @Rowern!

I think the right way to handle this is to call the destroy() method on the store, which already uses rimraf internally. See here: https://github.com/feross/fs-chunk-store/blob/8d7a57b56572625346024fca8cf3e2839ba2d49d/index.js#L222-L232

I haven't looked, but I suspect that we're only calling close() to close the store, but not destroy it.

You're right that we also want to keep the option to destroy the torrent without destroying the files on disk, so that probably should be an option to torrent.destroy(), like torrent.destroy({ remove: true }) which defaults to false.

@Rowern

This comment has been minimized.

Copy link
Author

Rowern commented Apr 13, 2017

You are right, I will continue to work on this PR tomorrow.

Add an optional `opts` to `client.remove` and `torrent.remove`,
if this `opts` set `remove` to `true`, then we call `store.destroy`
instead of `store.close`.

client.remove(torrent, {'remove': true}, function (err) {
t.error(err, 'torrent removed')
fs.stat(completeFileName, function (err) {

This comment has been minimized.

Copy link
@Rowern

Rowern Apr 14, 2017

Author

I know this might not work well with browser API, I might add a check on the type of store used.

@Rowern Rowern closed this May 3, 2017
@Rowern Rowern reopened this May 3, 2017
@Rowern

This comment has been minimized.

Copy link
Author

Rowern commented May 3, 2017

Sorry for closing/re-opening but it was the only way to re-trigger a travis build.

@stale

This comment has been minimized.

Copy link

stale bot commented Aug 2, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Aug 2, 2018
@stale stale bot closed this Aug 9, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Nov 7, 2018
@webtorrent webtorrent unlocked this conversation Aug 5, 2019
@feross feross reopened this Aug 5, 2019
@stale stale bot removed the stale label Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.