Add suspendables to throttler in consistent state #640
Conversation
matthias-wende-sociomantic
changed the title
|
|
||
| if (this.suspended_ && !s.suspended()) | ||
| { | ||
| s.suspend(); |
There was a problem hiding this comment.
should it be suspended if it isn't added to the suspendables? Anyway if there is any reason to do so then the documentation of the method should be updated.
There was a problem hiding this comment.
Yes that's the whole point. Recycled instances are in the list and they should be suspended.
There was a problem hiding this comment.
And good point. I updated the doc accordingly ;).
|
If this is a bug fix then it would be good to have/add a unit test |
matthias-wende-sociomantic
force-pushed
the
fix_throttler
branch
from
e29733c
to
12546c1
Compare
Yeah, thanks for pointing that out. I've added one for this particular case. |
|
Hmm wasn't the idea originally to add a method allowing the user to remove a suspendable from the throttler? |
|
Yes, that was the old plan before this new one :).
At the same time the original idea introduced some new regression to the client and I have |
|
You describe this situation:
What I'm concerned about is this: what happens if, after step 2, the throttler tries to suspend? It would be calling a method of the recycled suspendable, which would do...? The fact that invalid suspendables can (must, currently) stay registered with the throttler seems like a bug to me, not something that we should patch up to make work. What do you think? |
|
Note that a recycled invalid suspendable is only one case. What if the invalid suspendable has been garbage collected, say? |
matthias-wende-sociomantic
force-pushed
the
fix_throttler
branch
from
bc2fb17
to
00340a1
Compare
matthias-wende-sociomantic
changed the title
|
Yes that's a valid point a should be addressed as well. My description of this patch was somehow misleading. I've updated the PR and commit descriptions to what this patch is actually about. |
| suspendable_throttler.suspendAll(); | ||
|
|
||
| // add a resumed ISuspendable to a suspended throttler | ||
| // note that the instance is "recycled" |
There was a problem hiding this comment.
This one comment probably isn't needed any more.
| suspendable.suspend(); | ||
| suspendable_throttler.addSuspendable(suspendable); | ||
|
|
||
| test!("==")(suspendable.suspended(), false, "Unit test failed, " ~ |
There was a problem hiding this comment.
Generally, I don't think you need to write "Unit test failed". It's clear from the context.
| suspendable.resume(); | ||
| suspendable_throttler.addSuspendable(suspendable); | ||
|
|
||
| test!("==")(suspendable.suspended(), true, "Unit test failed, " ~ |
There was a problem hiding this comment.
Generally, I don't think you need to write "Unit test failed". It's clear from the context.
|
It seems like this should be a patch release. |
matthias-wende-sociomantic
force-pushed
the
fix_throttler
branch
from
00340a1
to
b278144
Compare
This change ensures that the suspend state of an ISuspendable instance when added to the throttler is consistent with the throttler state.
This test checks if an instance of ISuspendable that is added to the throttler has the same suspend state as the throttler.
matthias-wende-sociomantic
force-pushed
the
fix_throttler
branch
from
b278144
to
f103e06
Compare
|
Updated, base branch changed + rebased on top of new branch, accidentally deleted branch, recovered the branch and finally reopened the PR :). |
| else if (!this.suspended_ && s.suspended()) | ||
| { | ||
| s.resume(); | ||
| } |
There was a problem hiding this comment.
Why make it simple if we could make it complicated?
if (this.suspended_ != s.suspended())
{
if (this.suspended_)
s.suspend();
else
s.resume();
}There was a problem hiding this comment.
This PR has been merged, but that's a good point.
There was a problem hiding this comment.
Yep, that's looks indeed better.
| } | ||
| } | ||
|
|
||
| version (UnitTest){ |
|
|
||
| public bool suspended ( ) | ||
| { | ||
| return suspended_; |
This change ensures that the suspend state of an ISuspendable instance
when added to the throttler is consistent with the throttler state.
This patch somehow also works around the the following problem.
If a RequestSuspender instance is reused by a new Request it'll set the suspend_requested flag to false.
https://github.com/sociomantic-tsunami/dhtproto/blob/v14.x.x/src/dhtproto/client/legacy/internal/request/model/IBulkGetRequest.d#L133
https://github.com/sociomantic-tsunami/swarm/blob/v5.x.x/src/swarm/client/request/helper/RequestSuspender.d#L148
When adding this instance to the throttlers list of suspendables it will be already present and therefore neither appended to the list of suspendables nor suspended.
https://github.com/sociomantic-tsunami/ocean/blob/v4.x.x/src/ocean/io/model/ISuspendableThrottler.d#L77
Now if at the same time the application (i.e. the applications throttler) is in suspended state, the request will be started in a non suspended state and will not get suspended at least until the throttler resumes and suspends again.
https://github.com/sociomantic-tsunami/ocean/blob/v4.x.x/src/ocean/io/model/ISuspendableThrottler.d#L139
Note that that doesn't address the issue that a suspendable is kept in the list of suspendables when a request finishes (compere also the comment below). The possibility for clients to unregister suspendables from the Throttller when a request finishes will be addressed later.