Replace AsyncProcess exit handler by weakref.finalize

[pentschev]

This is an attempt at solving #4181 . I believe this is a more reliable approach, given we can ensure the process will not get destroyed before it's garbage collected.

[quasiben]

This does resolve a long standing cleanup issue for UCX and we think this is a cleaner approach to halt workers. @jcrist if you have time can you look this over ?

[quasiben]

@fjetter your thoughts here would also be helpful

[jcrist]

A few quick comments. No thoughts on whether this is the right approach over other methods - generally I'd hope we could explicitly manage closing these processes rather than requiring a finalizer to implicitly handle them.

[pentschev]

A few quick comments. No thoughts on whether this is the right approach over other methods - generally I'd hope we could explicitly manage closing these processes rather than requiring a finalizer to implicitly handle them.

I think the main problem we have today is that using exit handlers as globals as is the case in Distributed seems incorrect, as the order will not be respected. I haven't looked at all exit handlers but the few I looked at will all have the same problem, a couple examples:

distributed/distributed/client.py

Lines 4838 to 4850 in 18fff8b

	def _close_global_client():
	"""
	Force close of global client. This cleans up when a client
	wasn't close explicitly, e.g. interactive sessions.
	"""
	c = _get_global_client()
	if c is not None:
	c._should_close_loop = False
	with suppress(TimeoutError, RuntimeError):
	c.close(timeout=3)
	atexit.register(_close_global_client)

distributed/distributed/deploy/spec.py

Lines 636 to 641 in 18fff8b

	@atexit.register
	def close_clusters():
	for cluster in list(SpecCluster._instances):
	with suppress(gen.TimeoutError, TimeoutError):
	if cluster.status != Status.closed:
	cluster.close(timeout=10)

I don't know whether this is the right approach either, I was hoping to suggest this and begin a discussion on potential pitfalls and alternatives. At the moment I don't see a much better approach,

[jcrist]

Provided tests pass, this looks good to me.

[fjetter]

I agree with @jcrist and would prefer a world where we would not need to cleanup with finalizers but instead would be able to close everything intentionally.

However, as far as finalizers go I think this approach is slightly better than the one before since it deals with each object/process individually instead of in bulk. In particular, as already mentioned, it respects the ordering in the order the objects where created. Before the order was determined on module import time. Not sure if this makes any difference but this approach seems also simpler to reason about than the module level finalizer.

[fjetter]

There are a lot of failures on travis/py3.6. Most seem unrelated but I'm not entirely sure.

[pentschev]

I'm also not sure whether the 3.6 failures are related to this, it seems that they aren't, but happy to look more into that if there's some suspicion this PR is the cause.

[quasiben]

I restarted the 3.6 CI job

[pentschev]

Seems like there's still one test failing with a timeout: test_broken_worker_during_computation. By looking at builds from other PRs in https://travis-ci.org/github/dask/distributed/jobs/738345888 and https://travis-ci.org/github/dask/distributed/jobs/738412961, the same test is failing, so I believe it's not a side-effect of the changes here.

[fjetter]

The test_broken_worker_during_computation failure is already reported in #4173

[pentschev]

Thanks @fjetter , I hadn't seen that. In this case, I really think this PR seems safe.

[jrbourbeau]

Thanks @pentschev! (and @jcrist @fjetter for reviewing)

[pentschev]

Thanks everyone for reviews and merging! :)