Skip to content

Move all context-manager dependent returns within context manager scope

Yaroslav Halchenko requested to merge neurodebian-team/snapshot:bf-500s into master

My instance of the server (neurodebian snapshots) was bombarded with requests which lead to crash and thus 500.

Original exception:

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/flask/app.py", line 2447, in wsgi_app
	response = self.full_dispatch_request()
  File "/usr/lib/python3/dist-packages/flask/app.py", line 1952, in full_dispatch_request
	rv = self.handle_user_exception(e)
  File "/usr/lib/python3/dist-packages/flask/app.py", line 1821, in handle_user_exception
	reraise(exc_type, exc_value, tb)
  File "/usr/lib/python3/dist-packages/flask/_compat.py", line 39, in reraise
	raise value
  File "/usr/lib/python3/dist-packages/flask/app.py", line 1950, in full_dispatch_request
	rv = self.dispatch_request()
  File "/usr/lib/python3/dist-packages/flask/app.py", line 1936, in dispatch_request
	return self.view_functions[rule.endpoint](**req.view_args)
  File "/home/snapshot/code/web/app/snapshot/lib/cache.py", line 34, in wrapped
	rv = f(*args, **kwargs)
  File "/home/snapshot/code/web/app/snapshot/views/archive.py", line 92, in archive_dir
	node = ArchiveController.get_node(archive, date, path)
  File "/home/snapshot/code/web/app/snapshot/controllers/archive.py", line 103, in get_node
	run = get_snapshot_model().mirrorruns_get_mirrorrun_at(archive, date)
  File "/home/snapshot/code/web/app/snapshot/models/snapshot.py", line 132, in mirrorruns_get_mirrorrun_at
	if len(rows) != 0:
UnboundLocalError: local variable 'rows' referenced before assignment

and looking at the code, it is apparently a generic pattern to have

with DBInstance(...) as db:
    var = ... query ...
return f(var)

which results in "var" not defined if context never entered for some reason. Some functions has it as

var = None
with DBInstance(...) as db:
    var = ... query ...
return var

but not generic enough, and I thought that it would be safer and more generic to just rely on the fact that function without explicit return would return None anyways, so we would be safer to remain within context block.

For my specific use case above, I do get now 404 "No mirrorrun found at this date." and similar request on https://snapshot.debian.org/archive/debian%C0%A7%C0%A2%252527%252522%5C%27%5C%22/20101014T200503Z/changelogs/pool/main/ results in 500.

Merge request reports