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

Reallow to pause checking torrents #11354

Merged
merged 1 commit into from Nov 9, 2019

Conversation

@thalieht
Copy link
Contributor

thalieht commented Oct 10, 2019

Revert 87d9840

Closes #11076, closes #11400

@Chocobo1 Chocobo1 added this to the 4.2.0 milestone Oct 10, 2019
@Chocobo1 Chocobo1 added the Core label Oct 10, 2019
@glassez

This comment has been minimized.

Copy link
Member

glassez commented Oct 24, 2019

@thalieht, are you test it? Couldn't it is possible lead to an inconsistent state?

What does happen, e.g., if you pause "checking paused" torrent? And then resume it?

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Oct 24, 2019

What does happen, e.g., if you pause "checking paused" torrent? And then resume it?

If by "checking paused" you're not referring to some weird internal state but to pause->recheck->interrupt recheck by pausing the torrent->resume, then yeah i had tested it and it behaves as expected: it resumes rechecking where it left off. However after qBt restart, the torrent will automatically recheck if the recheck wasn't finished. Not sure if that's expected behavior (don't know where to look in order to change it, if it isn't).

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Oct 28, 2019

It isn't good idea to pause checking process in any case. Well, user can have reason to do it so I will discuss no pros and cons.

First case is when the user triggers "force recheck" of paused torrent manually. Seems we can easily allow to pause it during checking but we need to clear m_pauseWhenReady flag.
The second case is when user wants to pause torrent that is checking during startup. This is a complex case and it requires careful research, otherwise we can plunge the torrent into inconsistent state.

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Oct 28, 2019

The second case is when user wants to pause torrent that is checking during startup. This is a complex case and it requires careful research, otherwise we can plunge the torrent into inconsistent state.

I bet it involves m_startupState, i can't understand it. Maybe you'd like to take over? Turns out it's not as simple as i thought...

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Oct 29, 2019

I bet it involves m_startupState, i can't understand it. Maybe you'd like to take over?

At least you can finish the first case here. It is easy as you can see from my comment. And it should be done before v4.2 release.
I have no time now. I don't even have a suitable computer at my disposal.

@thalieht thalieht force-pushed the thalieht:pausechecking branch from 86f92db to 0fe3f4a Oct 29, 2019
@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Oct 29, 2019

At least you can finish the first case here.

Done (i hope) with some rudimentary testing again, no problems found.

if (isPaused()) return;
if (isChecking())
m_pauseWhenReady = false;

This comment has been minimized.

Copy link
@glassez

glassez Oct 29, 2019

Member

Well, I've just done some code review. This flag is cleared in handleTorrentPausedAlert() so it seems you don't really need to clear it here.

This comment has been minimized.

Copy link
@thalieht

thalieht Oct 29, 2019

Author Contributor

Reverted.

@thalieht thalieht force-pushed the thalieht:pausechecking branch from 0fe3f4a to cb887f4 Oct 29, 2019
@@ -1484,12 +1484,6 @@ void TorrentHandle::toggleFirstLastPiecePriority()
void TorrentHandle::pause()
{
if (m_startupState != Started) return;

This comment has been minimized.

Copy link
@glassez

glassez Oct 29, 2019

Member

Seems to handle the second case we just need to set m_startupState to Started under this condition.

This comment has been minimized.

Copy link
@thalieht

thalieht Oct 29, 2019

Author Contributor
 void TorrentHandle::pause()
 {
-    if (m_startupState != Started) return;
     if (isPaused()) return;
 
+    m_startupState = Started;
     setAutoManaged(false);
     m_nativeHandle.pause();

or

 void TorrentHandle::pause()
 {
-    if (m_startupState != Started) return;
+    if (m_startupState != Started) {
+        if (isChecking())
+            m_startupState = Started;
+        else
+            return;
+    }
     if (isPaused()) return;
 
     setAutoManaged(false);

This comment has been minimized.

Copy link
@glassez

glassez Oct 29, 2019

Member
if (m_startupState != Started)
    m_startupState = Started;

But, seems, you need to move it to the end of the method.

This comment has been minimized.

Copy link
@thalieht

thalieht Oct 29, 2019

Author Contributor

Done, thanks.

This comment has been minimized.

Copy link
@glassez

glassez Oct 29, 2019

Member

👍
Now can you cause "checking on startup" case for some paused torrent to test it? Probably you need to trigger rechecking of some paused torrent and then close the app before it's done.

This comment has been minimized.

Copy link
@thalieht

thalieht Oct 29, 2019

Author Contributor

When i do it to a seeding torrent, recheck->close qBt before it finishes->start qBt and pause rechecking torrent->resume/force resume
After the recheck finishes, the torrent becomes paused. Shouldn't it go back to seeding?

This comment has been minimized.

Copy link
@glassez

glassez Oct 30, 2019

Member

Shouldn't it go back to seeding?

Sure.

recheck->close qBt before it finishes

You don't pause it before recheck? Then it is a different kind of (related) issue.

This comment has been minimized.

Copy link
@thalieht

thalieht Oct 30, 2019

Author Contributor

Rechecking paused torrent doesn't have any problems, works as expected.

This comment has been minimized.

Copy link
@glassez

glassez Oct 30, 2019

Member

Rechecking paused torrent

We do talk about ability to pause (and then resume) checking torrents, don't it? So starting state is "checking" torrent.
Really we have 4 starting (sub)states that should be tested:

  1. Forced checking torrent (torrent was resumed when user triggers force recheck);
  2. Forced checking paused torrent (torrent was paused when user triggers force recheck);
  3. Startup checking torrent (torrent is added/restored as resumed, e.g. it was resumed when app is closed last time);
  4. Startup checking paused torrent (torrent is added/restored as paused, e.g. it was paused when app is closed last time).

In any of these cases the torrent should remain resumed after checking has done when user (pauses and then) resumes it explicitly.

@thalieht, do you understand what I'm talking about? If so can you please test and report all of these cases above?

This comment has been minimized.

Copy link
@thalieht

thalieht Oct 30, 2019

Author Contributor

In any of these cases the torrent should remain resumed after checking has done when user (pauses and then) resumes it explicitly.

I thought the idea was to return it to its prior state when manually resuming the torrent we paused during recheck, but if that's how it should be then only the 1st case resumes the torrent, in all the other cases the torrent gets paused after the recheck.

@thalieht thalieht force-pushed the thalieht:pausechecking branch from cb887f4 to f083073 Oct 29, 2019
@glassez

This comment has been minimized.

Copy link
Member

glassez commented Oct 30, 2019

I thought the idea was to return it to its prior state when manually resuming the torrent we paused during recheck

Hmm, interesting point of view... can you explain what it's based on?
Mine is based on the fact that the user's last explicit action is "resume torrent", so user expects the torrent will keep resumed.
Or you separate "checking" from all other torrent jobs so "pause" and "resume" when torrent is checking affects only "checking" process?

@xavier2k6

This comment has been minimized.

Copy link

xavier2k6 commented Oct 30, 2019

#11280 (comment)

some torrents seem to need to be rechecked over & over again on each new openeing of qbittorrent......although it seems the amount is becoming less & less......I believe it is to do with the "not responding" being encountered on checking said torrents.

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Oct 30, 2019

Or you separate "checking" from all other torrent jobs so "pause" and "resume" when torrent is checking affects only "checking" process?

I hadn't given it any thought but it's pretty much what you said. It felt more intuitive. Also whether you were sarcastic or actually asking for my opinion the answer to both is that i don't care either way and my POV is clearly subjective hence you are right that it should resume the torrent.

Mine is based on the fact that the user's last explicit action is "resume torrent"

"resume" actually :^)

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Oct 31, 2019

I have now given it some thought and stand by what i said that it should return to its prior state. Consider this: someone has his torrents on an external drive and forgets to connect it before starting qBt. They all start rechecking. "damnit not now i have work to do" and pauses everything. He had a mix of paused and non-paused torrents and i doubt he wants everything resumed when rechecking is finished. Remember, recheck was issued automatically/forcefully/unwillingly and the pause/resume commands were manual and explicit during the recheck. So despite what the code says, has the human given the command to "resume torrent"?

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Oct 31, 2019

Also whether you were sarcastic or actually asking for my opinion

No sarcasm.
I am a supporter of simple, strict and reliable interfaces. But it often requires a certain skill and/or mentality from the user, so it is inconvenient for most ordinary users. So I'm interested in a different opinion.

someone has his torrents on an external drive and forgets to connect it before starting qBt.

This isn't so good example. Such torrents just become "missing files". But you are right generally: recheck can be issued automatically/forcefully/unwillingly in many cases.

the pause/resume commands were manual and explicit during the recheck

So you still lead to the fact that "checking" is a special state that can be started/paused regardless of the main state of the torrent. Hmm... In principle, I do not object to such a model, if we manage to formalize it consistently. The question is, will it be intuitive to the user that these actions (pause/resume) are context sensitive?

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Oct 31, 2019

some torrents seem to need to be rechecked over & over again on each new openeing of qbittorrent......

Sorry, it seems like a different issue.

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Oct 31, 2019

@jagannatharjun, @Chocobo1, can you share the discussion above (about special meaning of "checking" state)?

@jagannatharjun

This comment has been minimized.

Copy link
Contributor

jagannatharjun commented Oct 31, 2019

Well, I didn't know the internals of qBittorrent yet. So I don't think I can add anything useful here
But by reading the comments and with my little understanding, I want to say that the idea should be that one can pause a torrent at any point in time, and "Resuming" should resume the torrent i.e Seeding if it were seeding previously or checking if it were checking.

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Oct 31, 2019

@jagannatharjun the question is: if a paused torrent (initial state) started rechecking but was paused before it finishes, when user resumes the rechecking should the torrent go back to paused or resumed (seeding/downloading etc) when it finishes?

@Chocobo1

This comment has been minimized.

Copy link
Member

Chocobo1 commented Oct 31, 2019

@jagannatharjun the question is: if a paused torrent (initial state) started rechecking but was paused before it finishes, when user resumes the rechecking should the torrent go back to paused or resumed (seeding/downloading etc) when it finishes?

I would expect resumed state.

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Oct 31, 2019

I would expect resumed state.

Do you clearly understand the reasons for the contrary opinion?

@xavier2k6

This comment has been minimized.

Copy link

xavier2k6 commented Nov 1, 2019

some torrents seem to need to be rechecked over & over again on each new openeing of qbittorrent......

Sorry, it seems like a different issue.

All ok now......had an extra resume file in BT_backup folder from about 2 yrs ago that had no torrent or torrent file associated with it....deleted it & resumed all other files manually for a split second or two in qbittorrent/re-paused - all saved same modified date/time regarding fastresume file & has not been stuck in a re-check loop on every start-up since.

@Chocobo1

This comment has been minimized.

Copy link
Member

Chocobo1 commented Nov 1, 2019

Do you clearly understand the reasons for the contrary opinion?

I haven't look into much, that viewpoint was made as an user.

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Nov 1, 2019

I haven't look into much, that viewpoint was made as an user.

Ok, I asked for user opinions.

@LordNyriox

This comment has been minimized.

Copy link
Contributor

LordNyriox commented Nov 1, 2019

@thalieht:

if a paused torrent (initial state) started rechecking but was paused before it finishes, when user resumes the rechecking should the torrent go back to paused or resumed (seeding/downloading etc) when it finishes?

IMO, the "initial" state should be preserved, assuming it is reasonable to do so. I like to micromanage my qBittorrent transfers, which includes a lot of pausing and manual rechecking at various times.

If some emergency comes up, and I have to pause a running recheck process to free up system resources, then I would like to be able to go back to exactly what I was doing in qBittorrent once the emergency is resolved.

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Nov 1, 2019

Guys, it seems that you still do not understand what the hitch is...
Although the purpose of the changes is stated as "allow to pause checking torrents", it is not a problem in itself: just remove a couple of lines of code. The problem is what should happen next when we resume it again.
The fact is that "checking" can be performed for generally paused torrent (for which it temporarily resumes and then pauses again), for example, this can happen when a previously paused torrent is starting up, or when the user manually rechecks some paused torrent.
An illustrative example is #11076. Let's imagine you want download some torrent and run qBittorrent but it starts to check existing (both paused and resumed) torrents for some reason. You don't want to wait for it to finish checking and pause them all. After you've done your current job you want to resume all existing torrents. What should you do? And what behavior do you expect from qBittorrent?

@@ -1500,6 +1493,9 @@ void TorrentHandle::pause()
// We test on the cached m_nativeStatus
if (isQueued())
m_session->handleTorrentPaused(this);

This comment has been minimized.

Copy link
@glassez

glassez Nov 1, 2019

Member

@thalieht, can you add here the following:

if (m_pauseWhenReady) {
#if (LIBTORRENT_VERSION_NUM < 10200)
        m_nativeHandle.stop_when_ready(false);
#else
        m_nativeHandle.unset_flags(lt::torrent_flags::stop_when_ready);
#endif
        m_pauseWhenReady = false;
}

and check 4 cases we discussed earlier?

This comment has been minimized.

Copy link
@thalieht

thalieht Nov 1, 2019

Author Contributor
  1. Resumed (initial Resumed)
  2. Resumed (initial Paused)
  3. Paused (initial Resumed on startup)
  4. Resumed (initial Paused on startup)

But

-  if (m_pauseWhenReady) {
+  if (!m_pauseWhenReady) {
   #if (LIBTORRENT_VERSION_NUM < 10200)
       m_nativeHandle.stop_when_ready(false);
   #else
       m_nativeHandle.unset_flags(lt::torrent_flags::stop_when_ready);
   #endif
-      m_pauseWhenReady = false;
   }
  1. Resumed (initial Resumed)
  2. Paused (initial Paused)
  3. Resumed (initial Resumed on startup)
  4. Paused (initial Paused on startup)

Have we decided whether it should or shouldn't resume in all cases?

This comment has been minimized.

Copy link
@glassez

glassez Nov 2, 2019

Member

Have we decided whether it should or shouldn't resume in all cases?

Still no.
But we can find both solutions and then just apply the chosen one.

This comment has been minimized.

Copy link
@glassez

glassez Nov 2, 2019

Member

Can you also test the following?

#if (LIBTORRENT_VERSION_NUM < 10200)
        m_nativeHandle.stop_when_ready(false);
#else
        m_nativeHandle.unset_flags(lt::torrent_flags::stop_when_ready);
#endif
        m_pauseWhenReady = false;

This comment has been minimized.

Copy link
@thalieht

thalieht Nov 2, 2019

Author Contributor

Resumes in all cases.

This comment has been minimized.

Copy link
@glassez

glassez Nov 6, 2019

Member

Well, since no one is active in the survey, I choose "return to previous state" (as the path of least resistance). We can change this behavior later if there are user requests.
I rethought the solution once again: seems you should just revert entire 87d9840 plus remove if (m_startupState != Started) return; from TorrentHandle::resume().

@thalieht thalieht deleted the thalieht:pausechecking branch Nov 9, 2019
@glassez

This comment has been minimized.

Copy link
Member

glassez commented Nov 9, 2019

if (isPaused() && (m_startupState == Started))

Yes, but in reverse order.

it pauses the moment it starts checking the "on startup checking torrent i paused" that i Force recheck.

Sorry, I cannot understand what do you mean.

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Nov 9, 2019

I'll just make another commit for it

Backport PR that contains commits from several upstream PRs is allowed.

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Nov 9, 2019

Sorry, I cannot understand what do you mean.

Cases 3 and 4 (rechecks on startup, both originally resumed and paused)

Instead of Resuming the checking torrent that i paused, i Force recheck. It goes to 0.0% and is paused immediately.

@thalieht thalieht referenced this pull request Nov 10, 2019
@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Nov 10, 2019

#11472. I just removed the first line for now until there is an answer.

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Nov 10, 2019

Well, I didn't think it through. It is necessary to begin with a questions "for what it is needed?" and "what the user expects as a result?". So please describe possible use cases.

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Nov 10, 2019

"for what it is needed?" and "what the user expects as a result?". So please describe possible use cases.

To be able to Force recheck in legitimate cases which i mentioned above.

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Nov 10, 2019

legitimate cases which i mentioned above.

What cases? You just meantioned that "Instead of Resuming the checking torrent that i paused, i Force recheck". But what's the reason to do it? And what do you expect as a result?
Seems there are two possible ways: restart "preparing" process or skip "preparing" process (if you understand what I mean).

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Nov 10, 2019

You just meantioned that "Instead of Resuming the checking torrent that i paused, i Force recheck". But what's the reason to do it?

Honestly i wouldn't do it but apparently it's not unexpected
#11354 (comment)

Press pause to interrupt it. Now user can have 2 actions to leave this paused state:
a. Select "recheck torrent" again, this action will go to step 1.
b. Select resume. This will recheck the torrent and then starts running, i.e. leaving paused state.

if you understand what I mean

Once again no, i don't know what "preparing", "starting" entails but i expect it to continue rechecking (if possible, currently it starts from the beginning) or to restart checking, as it currently does.

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Nov 15, 2019

It has already been explained several times... Well, I'll say it again. "Preparing" is "initial fastresume data/torrent checking" and analyzing its result.

But back to our question...
Some torrent can be started to become either paused or resumed (i.e. "target" state). You have paused its checking. What state it should reach after you perform force recheck on it? We have chosen "target" state in case you resume it. Also be noted that when you recheck paused (started) torrent it becomes paused again. So what behavior should be used in case of recheck of "preparing" torrents?

BTW, user can perform "recheck" when checking is being performed.

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Nov 15, 2019

What state it should reach after you perform force recheck on it?

Its previous state, "preparing" torrents included in the answer.

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Nov 16, 2019

So how it works in current #11472?

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Nov 16, 2019

Pauses in all cases...

if (isPaused()) {
#if (LIBTORRENT_VERSION_NUM < 10200)
m_nativeHandle.stop_when_ready(true);

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Nov 16, 2019

Can you test the following?

void TorrentHandle::forceRecheck()
{
    if (!hasMetadata()) return;

    m_nativeHandle.force_recheck();
    m_unchecked = false;

    if ((m_startupState != Started) || isPaused()) {
#if (LIBTORRENT_VERSION_NUM < 10200)
        m_nativeHandle.stop_when_ready(true);
#else
        m_nativeHandle.set_flags(lt::torrent_flags::stop_when_ready);
#endif
        setAutoManaged(true);
    }

    if ((m_startupState == Started) && isPaused())
        m_pauseWhenReady = true;
}
@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Nov 16, 2019

  1. Paused (initial Resumed)
  2. Paused (initial Paused)
  3. Resumed (initial Resumed on startup)
  4. Paused (initial Paused on startup)
@glassez

This comment has been minimized.

Copy link
Member

glassez commented Nov 16, 2019

Paused (initial Resumed)

It's really weird... Nothing should have changed for this case.

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Nov 16, 2019

isPaused() is true so it gets to m_nativeHandle.stop_when_ready(true);

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Nov 16, 2019

Then I don't understand first two cases...

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Nov 16, 2019

And maybe other cases too. What do you mean?

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Nov 16, 2019

Never mind, i don't understand how 3rd case works with these changes so... better disregard my speculations.

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Nov 16, 2019

Ah i get it, it's because m_pauseWhenReady = true was moved so the following is reached and it overrides m_nativeHandle.stop_when_ready(true)

resume_impl(m_needsToStartForced);

but that's not called when torrent is Started.

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Nov 16, 2019

@thalieht, I still don't understand you. What case we're discussing?

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Nov 16, 2019

1st and 3rd cases. Why 1st doesn't work and 3rd works.

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Nov 16, 2019

1st and 3rd cases. Why 1st doesn't work and 3rd works.

But what do you mean by the first case? You recheck resumed (i.e. non paused) torrent and it becomes paused after checking has done?

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Nov 16, 2019

You recheck resumed (i.e. non paused) torrent and it becomes paused after checking has done?

Yes. The 1st case is the only one that doesn't work the way we want i.e. to return to its prior state after the recheck.

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Nov 16, 2019

Yes. The 1st case is the only one that doesn't work the way we want i.e. to return to its prior state after the recheck.

But it shouldn't be affected by my changes and it should work as previously. It is "started" and "non paused" so it is out of both "if" bodies (in previous code it is out of "if" body so nothing is changed). Or am I wrong?

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Nov 16, 2019

and "non paused"

But it is paused in all cases when we issue the recheck. Also in previous code i said that it gets paused in all cases after the recheck.

You recheck resumed (i.e. non paused) torrent and it becomes paused after checking has done?

Wait.. i think there is a misunderstanding: In all cases we interrupt the checking by pause and then force recheck again. So in all cases the torrent is paused when we issue the second recheck.

@glassez

This comment has been minimized.

Copy link
Member

glassez commented Nov 16, 2019

So in all cases the torrent is paused when we issue the second recheck.

Then what's difference between 1st and 2nd cases?

Anyway, when we recheck paused torrent it should keep paused after checking has done (it is current behavior). When we recheck "preparing" torrent it should become in those state for which it is preparing (we suggest this behavior).

@thalieht

This comment has been minimized.

Copy link
Contributor Author

thalieht commented Nov 16, 2019

In that case your changes have the desired result. Considering what we were talking about in this issue i thought we were talking about the same thing, but instead of Resume as the final step we Force recheck.
Anyway i'll make PR for master with these changes as there is no time left for 4.2 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.