BUG: Normalize address before comparison

[TomAugspurger]

Closes #2058

[TomAugspurger]

cc @mrocklin

The test is good, but I'm not sure if this is the appropriate fix or not. My idea was to always normalize user-provided addresses. resolve_address seemed to fit the bill.

If you like the fix, then I'll go through worker.py and client.py to always resolve the address on user-defined input.

[mrocklin]

I don't have strong thoughts about how this could backfire. @pitrou is the expert here. I'm inclined to go ahead with it if the test suite doesn't complain.

[TomAugspurger]

I notice that other places accepting an address also take a resolve_address keyword argument. e.g. Scheduler.add_worker. I'll plan on following that approach. (the default it to resolve)

[mrocklin]

The windows failure is interesting. I haven't seen it before and may be related? I'm not sure.

[mrocklin]

I recommend avoiding the use of the normal context manager in async tests. If you were to use async with c that would work well, but we can't use that within tests that have to be within python 2 friendly files. Instead I recommend using try/finally.

Also, if you're not going to use the client created in the gen_cluster decorator then you probably want the following:

@gen_cluster()
def test_foo(s, a, b):
    c = yield Client(...)

    try:
        ...
    finally:
        yield c.close()

There are also some examples of this in test_client.py

[TomAugspurger]

OK, thanks. I was just using the context manager so that it was cleaned up.

[mrocklin]

Understood, but the context manager is synchronous API. There is no way for us to yield c.close() within the normal context mananger. I think that it just starts the close coroutine, but doesn't wait for it. You'll either need to use async with or explicitly call c.close() to be sure that it closes up.

[TomAugspurger]

Agreed that the windows test could be related. I'll see if it fails this time around.

[TomAugspurger]

Windows passed this time.

The Python 2 travis run failed: https://travis-ci.org/dask/distributed/jobs/394751774#L1637

[mrocklin]

Fine for now, but in the future I recommend foo.persist() over c.persist(foo)

[mrocklin]

Just for aesthetic reasons really