we might have a HostEntry in the dns cache already

[caolanm]

Change-Id: I2a08bd77f837ba3b8a2a57e90e79ce5bb86305e8

• Resolves: #
• Target version: master

Summary

TODO

• ...

Checklist

• I have run make prettier-write and formatted the code.
• All commits have Change-Id
• I have run tests with make check
• I have issued make run and manually verified that everything looks okay
• Documentation (manuals or wiki) has been updated or is not required

[Ashod]

Thanks, @caolanm. I like the direction, but have concerns with switching the callback. I think we should work towards a better design that handles this case seamlessly.

[Ashod]

We should check that origOnConnectFail is valid. There is no guarantee that it is.

[Ashod]

If we have to do this, something is likely wrong with the design. (Also see below.)

If we need this to call _response->finish();, then the question is: why isn't it called when the connection fails anyway?

I think we should call onDisconnect() (or its equivalent) from callOnConnectFail().

The original design is to always call the 'onFinished' handler. We promise this for the setFinishedHandler():

    /// Set a callback to handle onFinished events from this session.
    /// onFinished is triggered whenever a request has finished,
    /// regardless of the reason (error, timeout, completion).
    void setFinishedHandler(FinishedCallback onFinished) { _onFinished = std::move(onFinished); }

So it's a good idea to do so when we fail to connect as well. We can extend the Response::State enum to include ConnectionError (we can rename the current Error to ProtocolError), and always invoke the same onDisconnect() and onFinished, to let the client do whatever cleanup and/or continuation.

(I'm not sure we ever needed a 'connectFailHandler', as such, rather than AsyncConnectResult + onFinished. Perhaps what's missing is the new State above?)

[Ashod]

This isn't exception-safe. Yet another problem with the need to switch handlers, which we should avoid by having a more homogeneous design.

[caolanm]

My assumption here with "routing syncRequestImpl connect through asyncDNS" was that the remaining "sync" dns use in wsd/Storage.cpp StorageBase::validate might be amenable to being split up into something where the result in an async callback would be usable so all the remaining sync DNS uses could be dropped. That looks plausible for the uses in wsd/RequestVettingStation.cpp and wsd/wopi/WopiProxy.cpp, but the StorageBase::create use of validate looks much more uncertain to achieve successfully.

Trying an alternative approach of passing a HostEntry to "validate" and bubbling that down from the initial places the uri can come from becomes unwieldy pretty fast and terminates in a bunch of places where have the same issue.

So lets try another alternative.