Are explicit `drop`s necessary? about fantoccini HOT 9 CLOSED

I agree with you that having to call drop explicitly is pretty unfortunate. The reason it is currently needed is to avoid swallowing any errors. I could have the implementation of Drop resolve the fin future internally, which would in turn mean you can just let Client fall out of scope as normal, but this would also mean that if any errors occur when tearing down the connection, that error would either be hidden from the user, or cause a panic (if drop calls unwrap).

For what it's worth, the reason that Client has to be explicitly dropped is that it internally contains a reference-counted Inner (This is what allows Client to be Clone). Only when the last Client is dropped will Inner's destructor be called (which calls shutdown). If shutdown isn't called, then a) the WebDriver session isn't terminated, and b) fin would never resolve.

from fantoccini.

Pushed a proposed change in bd4c9d1 — how does that look to you?
See in particular:

from fantoccini.

This is definitely a usage improvement!

Is there machinery that prevents the fin future from being driven to completion (thus ending the session) before the actual work is completed?

from fantoccini.

This is definitely a usage improvement!

Great!

Is there machinery that prevents the fin future from being driven to completion (thus ending the session) before the actual work is completed?

Hmm, I'm not entirely sure what you mean here. I think what you're asking is why fantoccini can't just internally run the fin future to completion, and thus do the cleanup for the user? If so, the answer is that the library only has a Handle to the Tokio Core used to drive the HTTP connection to the WebDriver server. It can schedule work for that core using the handle, but it can't actually execute that work. Therefore, it becomes up to the caller (who has the Core) to use core.run() to complete the request. The library also cannot just create a new Core for doing cleanup, because the hyper::Client is already tied to the user's original Handle:

from fantoccini.

Sorry I was not very clear.

I meant "are we sure that the cleanup future won't execute until after all of the actual work* is done?"

from fantoccini.

Ah, I see. Technically, no. I don't think tokio/hyper gives guarantees about the order in which futures/requests are resolved. Given that they're all asynchronous, I suppose theoretically some existing WebDriver request could be lying around somewhere, not yet written to the wire, and then the shutdown command could be spawned on the core and race ahead of the existing command. But this sounds pretty unlikely to occur in practice to me. it's also not clear that we have a way to guard against this internally in the library, since we have no control over the execution of the futures that we return.

from fantoccini.

Publishing this now as 0.8.0, but open to further suggestions for how to improve the API :)

from fantoccini.

@jonhoo Still didn't get is there a way to make Client to close when exiting its scope?

from fantoccini.

No, sadly there is no such way since when a Client is dropped it is not able to execute any asynchronous code. We could perhaps check if there was still an active runtime, and if so, spawn the destructor, but that's itself pretty error-prone, and could result in some weird unexpected behavior. What we really want are Asynchronous Destructors, but those don't seem like they're going to be here any time soon.

from fantoccini.