Handle too many connections to Wayback automatically

[Mr0grog]

Long ago, we worked around an issue where we were getting lots of connection failures from Wayback with a dirty hack: if we ran out of retries but still had a failure to establish a new connection, we’d try resetting the whole session (only once!) and start over:

web-monitoring-processing/web_monitoring/cli.py

Lines 264 to 273 in 74b31b3

	except Exception as error:
	# On connection failures, reset the session and try again. If we
	# don't do this, the connection pool for this thread is pretty much
	# dead. It's not clear to me whether there is a problem in urllib3
	# or Wayback's servers that requires this.
	# This unfortunately requires string checking because the error can
	# get wrapped up into multiple kinds of higher-level errors :(
	if retry_connection_failures and ('failed to establish a new connection' in str(error).lower()):
	self.wayback.session.reset()
	return self.process_record(record)

At the time, I knew this was a kind of ugly hack, and probably masking some underlying issues. After some discussion with @danielballan and some spelunking through the source of requests and urllib3 today, we realized there were two problems:

1. We might be holding open connections for an unnecessarily long time. This can be fixed by explicitly closing our response objects.

   • @danielballan did this for WaybackClient.search in Explicitly close response when search is done. wayback#19

   • I need to do this when we load Mementos here in cli.py:

     web-monitoring-processing/web_monitoring/cli.py

     Lines 260 to 263 in 74b31b3

     	memento = self.wayback.get_memento(record.raw_url,
     	exact_redirects=False)
     	return self.format_memento(memento, record, self.maintainers,
     	self.tags)

     (Update: we reliably read the whole response, which closes the connection, so we are actually OK here.)

2. But that doesn’t cover everything. Ultimately, requests buries some features around connection pooling from urllib3 in a way that means each WaybackSession (we have one per thread, for 36 in production right now) could possibly create infinity connections and then hold onto up to 10. (so we could wind up attempting to hold 360 open connections to Wayback!) Some things we can do to be smarter about this:

   • Just reduce the number of threads in production! Already on this, but it doesn’t make the problem obvious or clear if you scale up too far. We should ultimately do better.

   • When get_memento() redirects, make sure we read the body and close the response before moving onto the next in the redirect chain. (Release connections for memento redirects wayback#20)

   • Automatically step down the number of memento threads (to a point) if one gets too many connections refused. (How Many is Too Many? Two? Five? Probably > 1 to differentiate between something spurious and Wayback actually telling us we have too many connections open. Then again, maybe the retry configuration can be partially responsible for this? Much nuance here.) We have to make sure we re-queue the CdxRecord that was being loaded in this case. The error we’d see to trigger this would include Failed to establish a new connection: [Errno 111] Connection refused

     (Update: I prototyped this out and it didn’t actually improve things over just sharing a connection pool between threads. See Use a shared HTTPAdapter across all threads #551 for details.)

   • Expose ways to set pool_maxsize and pool_block on the requests.HTTPAdapter instances used by WaybackSession so users can control this better. (Update: see Use a shared HTTPAdapter across all threads #551 and Sketch out a way to support multithreading wayback#23)

   • Make sure WaybackClient and WaybackSession and requests.Session and requests.HTTPAdapter are thread-safe, and use one client with appropriate pool_maxsize and pool_block parameters on its HTTPAdapter instead of one client per thread. This is the most resilient solution, but most questionable: we can make our stuff thread-safe by fixing DisableAfterCloseSession, but there seems to deep confusion among both users and maintainers of requests as to its thread-safety (urllib3 is all good).

     (Update: Given the issues in the requests package, see Use a shared HTTPAdapter across all threads #551 for a practical, short-term workaround in this repo and Sketch out a way to support multithreading wayback#23 for one approach to solving this better.)

[Mr0grog]

(Other options on the last point above include dropping requests and using urllib3 directly, but there are potentially lots of little edge cases that makes harder or leaves totally unknown.)

[Mr0grog]

All the work that is unique to this repo has now been done! Ideally we want to get thread-safety for Wayback in edgi-govdata-archiving/wayback#23 (or some flavor of it) done soon, and we’ll then update code here to make use of it.

[Mr0grog]

Update: over on the Wayback side, edgi-govdata-archiving/wayback#52 is about to land. It's the last stepping stone required before we can switch away from requests to make it thread-safe.