Protect the cache lock better #3604
Conversation
dbutenhof
added
bug
Server
API
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
PBENCH-1317 We found a case where a cache lock could "leak" when an error occurs reading a file in the visualize and compare APIs. The file read has now been repackaged with a `finally` to be sure the stream is closed and unlocked on error.
| datasets = params.query.get("datasets") | ||
| benchmark_choice = None | ||
| benchmark = None | ||
| for dataset in datasets: |
There was a problem hiding this comment.
While initializing benchmark here certainly doesn't hurt anything, I don't see how it helps anything, either: it is written unconditionally at line 113 before it is tested at line 116, so the initial value is always overwritten (and, benchmark is never read if we don't enter the loop).
There was a problem hiding this comment.
vscode was unreasonably paranoid about the reference outside the loop, and I pre-declared the thing just to shut it up.
There was a problem hiding this comment.
Curious: I didn't see the reference outside the loop (otherwise, I probably would have ratified the paranoia). Still, I don't object to shutting up the linter (I think that's the better alternative to trying to ignore it).
There was a problem hiding this comment.
Oh, here it is. Yeah, that reference probably should have been changed to benchmark_choice, rather than expanding the scope of benchmark....
PBENCH-1317
We found a case where a cache lock could "leak" when an error occurs reading a file in the visualize and compare APIs. The file read has now been repackaged with a
finallyto be sure the stream is closed and unlocked on error.