Support launching multiple Chrome DriverSessions

[illicitonion]

I have a test for this behaviour, but I'll wait for #14
to merge before adding it.

[fluffysquirrels]

Supporting multiple Chrome DriverSessions certainly sounds good!

I think just add the test to this PR and we can patch it slightly as required by #14. Without the test I'm having a little difficulty seeing how this impl supports multiple sessions.

EDIT: Ignore this, see my follow up comment.

Also, can you try a generic impl for Arc? Something like:

impl<T: Driver> Driver for Arc<T> {
    fn url(&self) -> &str {
        self.deref().url()
    }
}

Don't bother if it's tricky to get that to compile. I sometimes have problems when I try this sort of thing and from memory it may require specialization (default keyword) or more features to land in the compiler (tracking issue, RFC).

[fluffysquirrels]

So the reason you can't do this currently is the Driver::session(self) method consumes the ChromeDriver, right? How about just adding an explicit new session method to ChromeDriver that takes &self? I think that might be a bit more obvious from the rustdoc than some impl for Arc<ChromeDriver>.

I have a feeling that just adding ChromeDriver::session(&self) would work (by shadowing the method on the trait), but that might be a bit unusual and hence confusing. How about multiple_session? Happy to hear better suggestions :)

[fluffysquirrels]

Seeing the test helps, thanks! I still think ChromeDriver::multiple_session() might be clearer. The method could also be on a new trait MultiSessionDriver: Driver, which ChromeDriver would impl and others may also impl later.

[illicitonion]

I've got this working in a way I'm happy with, but need to do a little clean-up on it - will update this PR soon!

[illicitonion]

Updated, please take a look :)

[fluffysquirrels]

Hey, thanks for this. I'll take a look soon.

[equalsraf]

Personally I would avoid the hard dependency on Arc in the method return type. It seems it should be possible to implement the Driver trait for Arc and Rc

impl Driver for Arc<ChromeDriver> {
    fn url(&self) -> &str {
        &self.url
    }
}

with this impl alone it should be possible to launch multiple sessions (the same applies to std::rc::Rc)

#[test]
fn launch_multiple_chrome_sessions() {
    ensure_logging_init();
    use std::sync::Arc;
    let server = FileServer::new();
    let chromedriver = Arc::new(ChromeDriver::build().spawn().unwrap());

    let session1 = Arc::clone(&chromedriver).session(&Default::default()).expect("Error starting first browser");
    let session2 = chromedriver.session(&Default::default()).expect("Error starting second browser");

    let page1 = server.url("/page1.html");
    let page2 = server.url("/page2.html");
    session1.go(&page1).expect("Error getting page1");
    session2.go(&page2).expect("Error getting page2");

    assert_eq!(&session1.get_current_url().expect("Error getting url 1"), &page1);
    assert_eq!(&session2.get_current_url().expect("Error getting url 2"), &page2);
}

It is not as ergonomic as yours but it shifts the ability to do this to the caller,
by implementing the Driver trait to his own types (that wrap ChromeDriver). We can and should provide implementations for std types when applicable - e.g. Arc and Rc.

I dont know if you choose Arc because you intended to use this accross threads, but neither your approach or mine are sufficient to move the session into the closure e.g.

    thread::spawn(move || {
        chrome1.go(&page1).expect("Error getting page1");
    });

this fails with

error[E0277]: the trait bound `std::ops::Drop + 'static: std::marker::Send` is not satisfied
   --> tests/webdriver_client_integration.rs:352:5
    |
352 |     thread::spawn(move || {
    |     ^^^^^^^^^^^^^ `std::ops::Drop + 'static` cannot be sent between threads safely
    |
    = help: the trait `std::marker::Send` is not implemented for `std::ops::Drop + 'static`
    = note: required because of the requirements on the impl of `std::marker::Send` for `std::ptr::Uniqu
e<std::ops::Drop + 'static>`
    = note: required because it appears within the type `std::boxed::Box<std::ops::Drop + 'static>`
    = note: required because it appears within the type `webdriver_client::DriverSession`
    = note: required because it appears within the type `[closure@tests/webdriver_client_integration.rs:
352:19: 354:6 chrome1:webdriver_client::DriverSession, page1:std::string::String]`
    = note: required by `std::thread::spawn`

[illicitonion]

Yeah, that was my first implementation, but @fluffysquirrels didn't like the ergonomics of it as a caller…

…

On 3 Feb 2018 10:36, ***@***.***" ***@***.***> wrote: Personally I would avoid the hard dependency on Arc in the method return type. It seems it should be possible to implement the Driver trait for Arc and Rc impl Driver for Arc<ChromeDriver> { fn url(&self) -> &str { &self.url } } with this impl alone it should be possible to launch multiple sessions (the same applies to std::rc::Rc) #[test]fn launch_multiple_chrome_sessions() { ensure_logging_init(); use std::sync::Arc; let server = FileServer::new(); let chromedriver = Arc::new(ChromeDriver::build().spawn().unwrap()); let session1 = Arc::clone(&chromedriver).session(&Default::default()).expect("Error starting first browser"); let session2 = chromedriver.session(&Default::default()).expect("Error starting second browser"); let page1 = server.url("/page1.html"); let page2 = server.url("/page2.html"); session1.go(&page1).expect("Error getting page1"); session2.go(&page2).expect("Error getting page2"); assert_eq!(&session1.get_current_url().expect("Error getting url 1"), &page1); assert_eq!(&session2.get_current_url().expect("Error getting url 2"), &page2); } It is not as ergonomic as yours but it shifts the ability to do this to the caller, by implementing the Driver trait to his own types (that wrap ChromeDriver). We can and should provide implementations for std types when applicable - e.g. Arc and Rc. I dont know if you choose Arc because you intended to use this accross threads, but neither your approach or mine are sufficient to move the session into the closure e.g. thread::spawn(move || { chrome1.go(&page1).expect("Error getting page1"); }); this fails with error[E0277]: the trait bound `std::ops::Drop + 'static: std::marker::Send` is not satisfied --> tests/webdriver_client_integration.rs:352:5 | 352 | thread::spawn(move || { | ^^^^^^^^^^^^^ `std::ops::Drop + 'static` cannot be sent between threads safely | = help: the trait `std::marker::Send` is not implemented for `std::ops::Drop + 'static` = note: required because of the requirements on the impl of `std::marker::Send` for `std::ptr::Uniqu e<std::ops::Drop + 'static>` = note: required because it appears within the type `std::boxed::Box<std::ops::Drop + 'static>` = note: required because it appears within the type `webdriver_client::DriverSession` = note: required because it appears within the type ***@***.***/webdriver_client_integration.rs: 352:19: 354:6 chrome1:webdriver_client::DriverSession, page1:std::string::String]` = note: required by `std::thread::spawn` — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#17 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABFEuELfq9azHQmYyB3pbIKDg1jpatl4ks5tRKcTgaJpZM4R1L0J> .

[equalsraf]

Ah sorry i missed that.

I've been trying to figure out if it is possible to get Send.working but the only way I got it to work was by requiring it in the Driver definition.

pub trait Driver: Send {

which i dont think we should do.

[fluffysquirrels]

Send and Sync are normally derived if all fields on a struct support them, or you can manually implement them with an empty impl (they're just marker traits):

// unsafe because the compiler can't check your implementation is correct (or it would have
// derived it automatically for you).
unsafe impl Send for MySafeType {}
unsafe impl Sync for MySafeType {}

// You can also mark a type as unsafe for Send or Sync
impl !Send for MyUnSafeType {}
impl !Sync for MyUnSafeType {}

See the Rust nomicon page on Send and Sync

[fluffysquirrels]

You've made breaking changes to public types. These are expensive (force crate consumers to change) so we try to make them rarely and make sure it's worth it. Consequently we need to think hard about the API design.

I think this feature is worthwhile, but your current implementation has some rough edges and so have made a review with comments. Happy to discuss this further and to see any alternatives.

[fluffysquirrels]

Arc is really an implementation detail of ChromeDriver that we may want to change without impacting consumers. I don't see a reason to expose it outside the crate at all. If you want an Arc to know when to clean up (e.g. the chromedriver child process), go ahead and add it as a field on ChromeDriver.

Skimming ahead, I see you took my suggestion and made the MultiSessionDriver trait. This can create sessions without consuming the driver, so do we need to clone ChromeDriver at all?

[fluffysquirrels]

Unfortunately the github diff is a little messy here, but it looks like you've removed the consuming session method on Driver. I still see a benefit in having some method on Driver to create a session so that a crate user can create sessions polymorphically over Driver. The consuming method was perhaps a little biased towards the idiosyncrasies of geckodriver, which only supports a single session, so perhaps a redesign would be nice, but we still need something here.

[fluffysquirrels]

You've broken down a Driver instance here into its current important pieces: it supplies a URL and it implements Drop to support cleanup. When new facilities are added to Driver this method signature would also need to change. I prefer the OO style of passing an object and then all the method calls along the way can remain more stable.

I assume you made this change to try and work Driver not being clonable. Perhaps create_session should take a reference to Driver or Driver should be clonable. I haven't thought either option through yet, but perhaps they would give a neater design.

[illicitonion]

create_session can't take a reference, because the thing being referred-to is being moved into the method. I think that if we made Driver extend Drop, we could just pass in the Driver here, and we could call driver.url() in the function... Or having Driver extend Clone would do the job too...

[fluffysquirrels]

One option is that create_session doesn't have to move the Driver.

[fluffysquirrels]

Thought a bit more. I think I prefer a single instance of Driver creating multiple sessions, instead of cloning the driver. Also it's not so bad if GeckoDriver returns an error if a session has already been created. @equalsraf, does that make sense to you also?

So please try changing Driver::session to take a reference and DriverSession::create_session similarly.

[equalsraf]

Send and Sync are normally derived if all fields on a struct support them, or you can manually implement them with an empty impl (they're just marker traits):

My only point concerning sync/send was that send was not being automatically derived for Driver (or Drop + 'static) which means that choosing Arc seemed strange since we couldnt actually send it to another thread. Rc would make more sense, but ideally the caller would decide.

[illicitonion]

Yeah, the awkwardness is definitely that having a consuming session(self) makes it hard to have a non-consuming session(&self). If you're happy to just have a non-consuming session(&self), and have the GeckoDriver return an Err if you try to re-use it, that definitely works for me, though I can see the desire to model this lifetime in the types.

The awkwardness with that, though, is that it requires the caller to manually ensure that the Driver is kept alive while the DriverSession is. This comes for free using an Arc (or an Rc), but I believe would push work onto the caller to convince the borrow-checker that Driver doesn't drop while DriverSession is in scope.

This is why I find it somewhat nicer as a consumer to just be given an Arc<ChromeDriver> - it means that the underlying methods Just Work in all situations. While the API mandating an Arc is not ideal (being able to choose an Rc or a & would be nice), I find it hard to object to an Arc here on efficiency reasons, because necessarily when you're calling this method, you're forking a process, and the cost of an Arc is meaningless in that situation. I wonder whether I could put together something clever with AsRef... I'll give it a go :)

[fluffysquirrels]

The caller certainly doesn't need to handle a complicated type. No Arcs, no AsRefs, nothing clever. If you want to use those things, add them as fields in ChromeDriver.

Yes, an Arc is certainly efficient enough if you want to enable thread-safe use, which may be useful.

How about:

• Add an inner: Arc<ChromeDriverProcess> field on ChromeDriver.
• #[derive(Clone)] on ChromeDriver.
• Create session methods clone the ChromeDriver.
• Drop on ChromeDriverProcess cleans up the chromedriver process.

[illicitonion]

Moved this to #20

(But I believe the caller does need to know about some non-ChromeDriver type if we want to avoid the consumer having to clone the ChromeDriver)

[fluffysquirrels]

Continued in #20.