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

Inhibit system sleep while torrents are moving #18783

Merged
merged 5 commits into from
Apr 16, 2023

Conversation

Sentox6
Copy link
Contributor

@Sentox6 Sentox6 commented Mar 28, 2023

While a torrent is considered "finished" upon download completion, a session's handleTorrentFinished() event is only queued after any relevant move event is completed. So if a user chooses a post-download action such as system shutdown, torrents will finish, move, and then shutdown occurs.

However, inhibit system sleep behaviour is managed by an independent timer than checks the native status of torrents. This leads to a particular scenario where the user may expect sleep to be inhibited until downloads complete, followed by the system hibernating/shutting down/etc. But since sleep inhibition lapses as soon as torrents are downloaded, a move operation of sufficient duration may allow the system to suspend, meaning the expected post-download action is not executed.

I'm not sure this solution is ideal - for one, it may lead to a user-initiated location change inhibiting sleep unexpectedly. However, any other solutions would seem to require some architectural changes to how updatePowerManagement() works (i.e. transitioning from a timer to being event-driven) or a rethink of what constitutes "finished". Feedback appreciated.

Consider a moving torrent unfinished - prevents system suspension inhibition lapsing before torrents are considered 'complete' from a UI perspective.
@glassez
Copy link
Member

glassez commented Mar 29, 2023

I'm not sure this solution is ideal - for one, it may lead to a user-initiated location change inhibiting sleep unexpectedly.

I think any "moving files" is a valid reason for system sleep to be inhibited.

@glassez glassez added this to the 4.6.0 milestone Mar 29, 2023
@glassez glassez requested a review from a team March 29, 2023 04:04
Corrected boolean logic, since isFinished() being true was sufficient to return false regardless of isMoving().
@Sentox6
Copy link
Contributor Author

Sentox6 commented Mar 29, 2023

I think any "moving files" is a valid reason for system sleep to be inhibited.

Fair enough. I've updated the commit to correct the fix - the first attempt did not actually resolve the issue (my fault for not reading the boolean logic properly).

thalieht
thalieht previously approved these changes Mar 29, 2023
@glassez
Copy link
Member

glassez commented Mar 30, 2023

I think any "moving files" is a valid reason for system sleep to be inhibited.

Fair enough.

But your changes make it to behave inconsistently, don't they? Moving files will affect it only when Inhibit system sleep when torrents are downloading is set. So maybe inhibit system sleep unconditionally when torrents moving files? Or provide yet another option for it?
Really I feel moving files is so important be done without any interrupts.

@glassez glassez requested a review from thalieht March 30, 2023 08:59
@thalieht
Copy link
Contributor

Now that i think about it... doesn't the OS know files are moved? Doesn't it prevent sleep during that time?

@glassez
Copy link
Member

glassez commented Mar 30, 2023

Now that i think about it... doesn't the OS know files are moved? Doesn't it prevent sleep during that time?

IIRC, it does not matter what tasks are performed by certain processes, but only whether the system is used interactively or not. If the system is not used interactively for a while, then this is a reason to switch to sleep mode.

@Sentox6
Copy link
Contributor Author

Sentox6 commented Mar 30, 2023

IIRC, it does not matter what tasks are performed by certain processes, but only whether the system is used interactively or not. If the system is not used interactively for a while, then this is a reason to switch to sleep mode.

Correct. It makes sense, given that in a modern OS environment file IO is constantly occurring (although it can certainly be unintuitive, such as when you initiate a large file transfer in shell and come back to find your PC asleep).

So maybe inhibit system sleep unconditionally when torrents moving files? Or provide yet another option for it?
Really I feel moving files is so important be done without any interrupts.

When I made this PR, my goal was just to make the minimal changes required to avoid a specific scenario (having sleep inhibited during downloads, choosing a completion action such as shutdown, and the action not succeeding) since I felt that was the most likely situation where this would cause a problem. However, I do agree that a more consistent behaviour change would be better. I would tend towards preventing sleep during move operations entirely, rather than adding another option, but that's my personal preference.

I'm happy to work on implementing either solution depending on your thoughts.

@thalieht
Copy link
Contributor

I would tend towards preventing sleep during move operations entirely, rather than adding another option

👍

Sleep prevention timer now always runs; suspends sleep during torrent move operations.
@Sentox6 Sentox6 marked this pull request as ready for review April 3, 2023 17:43
@glassez glassez added the Core label Apr 7, 2023
@glassez glassez requested a review from a team April 13, 2023 16:36
src/gui/mainwindow.cpp Outdated Show resolved Hide resolved
Style correction

Co-authored-by: thalieht <thalieht@users.noreply.github.com>
src/gui/mainwindow.cpp Outdated Show resolved Hide resolved
Restored indentation

Co-authored-by: thalieht <thalieht@users.noreply.github.com>
@glassez glassez changed the title Prevent download sleep suspension from lapsing before torrent completion Inhibit system sleep while torrents are moving Apr 15, 2023
@glassez glassez merged commit bd31edd into qbittorrent:master Apr 16, 2023
@glassez
Copy link
Member

glassez commented Apr 16, 2023

@Sentox6
Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants