Repository navigation
Return last evaluated progress on lock timeout in RestrictionTrackers.getProgress - #40205
Conversation
|
R: @Abacn |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
|
cc/ @scwhittle as well |
| try { | ||
| updateProgressBlocking(); | ||
| } finally { | ||
| lock.unlock(); |
There was a problem hiding this comment.
We now have mixed calls of unlock() and lock.unlock(). Consider name the new unlock() method updateProgressAndUnlock() ?
There was a problem hiding this comment.
I didn't do this because updateProgressAndUnlock() implies that we're always updating progress, which we're not. But I see the downside, and I think I prefer your approach.
| delegate.checkDone(); | ||
| } finally { | ||
| lock.unlock(); | ||
| unlock(); |
There was a problem hiding this comment.
Do all 4 occurrences of lock.unlock() -> updateProgressAndUnlock(); changes really need to update the progress cache as part of their method calls?
There was a problem hiding this comment.
Probably not, but the needsProgressUpdate is super cheap, so I'd prefer to keep it.
| needsProgressUpdate = true; | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| return lastProgress; |
There was a problem hiding this comment.
Is this correct if a split has occurred?
lastProgress may have been in the context of before the split which might confuse the runner.
There was a problem hiding this comment.
We can probably just unconditionally update progress after a split to fix this.
…pdateProgressAndUnlock
scwhittle
left a comment
scwhittle
left a comment
There was a problem hiding this comment.
I think that this seems safe now.
….getProgress (#40205) * Return last evaluated progress on lock timeout in RestrictionTrackers.getProgress * Update progress before unlocking when needsProgressUpdate is set * Unconditionally update progress after trySplit and rename unlock to updateProgressAndUnlock
Previously,
RestrictionTrackerObserverWithProgress.getProgressonly usedlock.tryLockwith a timeout until initial progress was reported (!hasInitialProgress), falling back to an unbounded blockinggetProgressBlocking()call afterwards. If a subsequenttryClaimortrySplitheld the lock for a long duration,getProgress()would still block indefinitely.This change:
lastProgress(initialized toProgress.NONE) and always useslock.tryLock(timeOutSec, TimeUnit.SECONDS)ingetProgress.getProgressfails to acquire the lock, setsneedsProgressUpdate = trueand returnslastProgress.RestrictionTrackerObservermethods (tryClaim,currentRestriction,trySplit,checkDone), checksif (needsProgressUpdate)and callsupdateProgressBlocking()inside atry ... finally { lock.unlock(); }block so thatlastProgressis kept fresh and the lock is always released.