Error, the webdriver won't quit correctly.

[jbernard002]

I was testing the async api with a runtime support.
And a very strange behaviour appeared, the code bellow won't move out at the end of the block_on closure.

The issue has been tested with 0.8 and 0.9 version of thirtyfour, tokio 0.2.19 and using the selenium docker command given in the readme.

use thirtyfour::prelude::*;
use tokio;

fn main() -> WebDriverResult<()> {

    let mut runtime = tokio::runtime::Builder::new()
        .basic_scheduler()
        .enable_all()
        .build().unwrap();

    let result : WebDriverResult<()> = runtime.block_on(async {

        let caps = DesiredCapabilities::chrome();
        let driver = WebDriver::new("http://localhost:4444/wd/hub", &caps).await?;

        println!("{:?}", driver);
        Ok(())
    });

    println!("End");

    result
}

After some investigation, the problem seems to come from the function drop (webdriver.rs:159).

fn drop(&mut self) {
        if self.quit_on_drop && !(*self.session_id).is_empty() {
            if let Err(e) = block_on(self.quit()) {
                error!("Failed to close session: {:?}", e);
            }
        }
    }

For some unknown reason, the block_on(self.quit()) loops without returning.
I tried to explicitly call driver.quit() before the end of the closure, but it didn't solve the problem.

Thanks

[stevepryde]

I'm able to reproduce the issue using your code. Thanks for providing that.
It seems to work when using the threaded scheduler but not the basic one.
I wonder if the futures::block_on method is trying to spawn a new task rather than run the async block to completion inline.

I had a lot of trouble with this method since async destructors are not currently supported in the language. There is an interesting blog post about it here: https://boats.gitlab.io/blog/post/poll-drop/.

I have tried using a tokio runtime inside this drop() method but tokio complains about creating a runtime within a runtime, which seems like an arbitrary restriction to me but maybe there is a good reason for it. Previously I have used an async_std runtime to do block_on() here, which worked fine but feels nasty. I'm not sure what futures::block_on does, but that seems to be the issue here.

Are you able to use the threaded_scheduler as a workaround for now?

[stevepryde]

I've added another potential workaround - as of 0.9.1 you will be able to manually call driver.quit().await? and that should work fine.

[stevepryde]

v0.9.1 is released now. Let me know if this helps.

I will leave this issue open since the underlying bug is still not resolved. Perhaps someone can provide a cleaner solution for this.

[jbernard002]

I've update the code to version 0.9.1 and it's works fine.
Thanks a lots!

[stevepryde]

If someone has a potential solution for this, a PR would be most welcome. Thanks!

[TilBlechschmidt]

I've run into a similar issue with one of my projects before. Not sure if it is the best solution but I just tokio::spawn'ed a closure which then takes care of dropping/finalising the value. Sure it is not guaranteed to run until completion if the process exits before it is done, but it is definitely better than a dead-lock.

(In the end, I implemented a process-wide job system which handles finalising of jobs including dropping values but that is totally overkill and unviable here)

[stevepryde]

It should be possible to run a closure on a new executor and block, but I'm not sure why that doesn't work. It's possible that reqwest also spawns a new task internally? That would make it deadlock.

For now the workaround is to manually close the session just before the webdriver goes out of scope.

[stevepryde]

I've done some more digging on this. Spawning a new task won't help unless you allow enough time for tokio to run that task. If your WebDriver goes out of scope at the end of your main function, you're probably out of luck. I don't really like this approach because it's really unclear if/when the browser will actually close. There's no way to access the join handle even if you wanted to.

I've also tried:

• Creating another tokio runtime inside drop() - tokio prevents this
• Using async_std::task::block_on - deadlock
• Using futures::executor::block_on (the current approach) - deadlock

The deadlock only happens if using the basic (single-threaded) runtime. It may be related to the way reqwest manages its connection pool using spawned futures. I haven't tried any other async http clients.

The short story is that what we really want are async destructors, but who knows whether such a feature will ever exist in Rust.
For now, the workaround is to either use the multi-threaded scheduler or manually call WebDriver::quit().await.

[TilBlechschmidt]

The question we should probably ask ourselves here is whether a session should actively be closed when it is dropped.

• From a pure library usability standpoint? Probably.
• From a puristic standpoint? Likely not.

Let me explain: In general, I'd expect as few side-effects as possible when calling a function unless its signature or documentation makes it explicitly clear. The fact that a destructor performs asynchronous "side-effects" which — compounding the issue — make network calls, may be considered undesirable and somewhat of an anti-pattern.

Imagine PR #38 where a session is persisted to disk. This feature would be completely useless, when the destructor ALWAYS, inevitably, makes a network call to nuke the session.

So in that regard, I'd argue that it should be a deliberate act from the user to close the session and not something the destructor does.

---

Welcome to the flip side. From a "out-in-the-wild" perspective it obviously makes sense to automagically end the session when the linked object is dropped. Especially, since this tends to cause real headaches when people are using remote infrastructure for sessions and just occupy idle sessions because they forgot a driver.quit() (happens way too frequently, trust me).

So overall, the best middle ground might be to remove the driver.quit() from the impl Drop and instead just throw a big, red, blinking¹ warning in the users face which reads something like:

Session dropped without calling .quit(), I hope you know what you are doing

Additionally, we could add a section in the documentation of WebDriver and the quit function.

_{¹I may be exaggerating a bit, but you get the gist}

[stevepryde]

I only just read this now, but I have independently reached the same conclusion to remove the impl Drop entirely.
This change is already merged to main and the README has been updated. This will become v0.25.0 once the documentation is updated as you suggested.

I asked a question on the tokio repo yesterday regarding alternative solutions and came away from the discussion with the understanding that Rust's async really prefers you to be explicit about what you want to do and await any operations performed. This agrees with what you said about performing work such as network requests during a destructor being an anti-pattern. I see the point and it makes sense. It's just really uncomfortable to imagine trying to explain this to someone coming to Rust from another language. Then again, Rust often coerces you into certain patterns "for your own good" so maybe it's not that unusual.

I don't mind your suggestion about logging an error if WebDriver was dropped without calling quit(). That seems reasonable. Perhaps there also needs to be a way to silence the error in cases where it was intended? That can come later if people request it I guess. Adding documentation around this on the quit() method would also be worthwhile.

[stevepryde]

v0.25.0 released, which removes impl Drop. I haven't added any logging around forgetting to call quit(). I think that needs more discussion.

What should the message say? What log level? Is it really necessary?

[liranco]

In my opinion, I don't think it's necessary to log it. If someone will expect the browser to close he'll probably search for a solution, so maybe it's better to document it instead :)

[TilBlechschmidt]

I agree with the sentiment, especially because adding a log message would definitely require a method to suppress it for those few people intentionally omitting it.

However, especially for people new to Selenium, the fact that you explicitly have to quit the driver or else it will stick around for potentially a long time should not be neglected.

We should make sure that every example includes the explicit call to quit() with a comment above on why it is there. Additionally, a prominent section in the documentation (maybe even the readme) on why and when (basically always unless you exactly know what you are doing) you should call .quit() would be a good addition as well — especially since this explicit call is not required for every Selenium Library and some do it implicitly.

[stevepryde]

Documentation updated on main. Let me know if this is sufficient.

[liranco]

Looks good to me 👍

[yerke]

Drive by comment re the example in README: if driver.find() or elem_form.find() returns an error, then the driver.quit().await? will not be called. Maybe the example should be updated with a more robust approach?