Allow shared reqwest client configuration #357

finnefloyd · 2021-09-01T09:24:00Z

From Reqwest official documentation:

The Client holds a connection pool internally, so it is advised that you create one and reuse it.
You do not have to wrap the Client it in an Rc or Arc to reuse it, because it already uses an Arc internally.

This PR, use a shared reqwest client among all GooseUser and allow the user the configure the global client.

jeremyandrews · 2021-09-02T15:35:42Z

Merge conflict needs to be resolved

jeremyandrews · 2021-09-02T15:38:10Z

It's unclear to me if this will result in cookies and headers being shared between all GooseUsers, which would be undesirable.

… we can share the client across all running GooseUser

finnefloyd · 2021-09-05T15:54:23Z

It's unclear to me if this will result in cookies and headers being shared between all GooseUsers, which would be undesirable.

Goose point, I did not think about it and I'm not using cookies in my current load tests, I didn't detect it. I moved the cookies handling to the GooseUser so we can shared the connection pool

jeremyandrews · 2021-09-06T05:53:54Z

This addresses cookies, but there’s still a problem from shared headers. For example, if using http header authentication, with this PR once one GooseUser logs in all would be.

jeremyandrews · 2021-09-06T06:03:29Z

For example, as documented here it's currently possible to set different headers with different GooseUsers, and this is required functionality for many load tests:
https://docs.rs/goose/*/goose/goose/struct.GooseUser.html#example-18

jeremyandrews · 2021-09-06T06:28:37Z

This example will also need to be updated to show how to set custom cookies:
https://docs.rs/goose/*/goose/goose/struct.GooseUser.html#example-19

Remove set_client_builder from GooseUser API. Rename set_client to set_client_builder on GooseAttack

finnefloyd · 2021-09-06T08:42:20Z

I added support for default headers and custom cookies to the GooseUser.

jeremyandrews

What is the intent of this change? Why are you trying to share a single Client pool?

Goose intentionally maintains a unique client-per-GooseUser, with the intent of better simulating independent users -- sharing a Client pool adds complexity and I worry this could result in unintended bugs. It also further ties Goose to the inner-workings of Reqwest, whereas the goal is to enhance Goose to support other client libraries and be less-tied to specific Reqwest implementation details.

I imagine there could be reduced memory requirements through this change, and possibly better startup-performance? (I am on the road an unable to test/validate this, which seems to be the only potential advantage of this added complexity?)

At this time I do not see sufficient value in merging this PR.

jeremyandrews · 2021-09-07T06:13:39Z

src/goose.rs

@@ -853,22 +853,21 @@ pub struct GooseUser {
    pub(crate) task_name: Option<String>,

    session_data: Option<Box<dyn GooseUserData>>,
+
+    cookie_store: Jar,


Please add comments to the new fields (it seems this was also missed for the new session_data; we can address that in a followup PR)

jeremyandrews · 2021-09-07T06:16:15Z

src/goose.rs

@@ -1582,6 +1586,12 @@ impl GooseUser {
            self.weighted_users_index,
        );

+        // Set cookies


For consistency add a . to the end of the comment.

jeremyandrews · 2021-09-07T06:16:21Z

src/goose.rs

+        // Set cookies
+        self.add_cookie_header(&mut request);
+
+        // Set default headers


For consistency add a . to the end of the comment.

jeremyandrews · 2021-09-07T06:18:20Z

src/goose.rs

+    }
+
+    // insert default headers in the request headers
+    // without overwriting already appended headers.


For consistency, please start comment with a capital letter. Also, the line-break in the comment should come after appended instead of after headers.

jeremyandrews · 2021-09-07T06:48:02Z

src/goose.rs

-    ///
-    /// In the following example, the Goose client is configured with a different user agent,
-    /// sets a default header on every request, stores cookies, and supports gzip compression.
+    /// Custom cookies can be manually set to each [`GooseUser`](./goose/struct.GooseUser.html).


set on instead of set to

jeremyandrews · 2021-09-07T06:59:54Z

src/goose.rs

-    ///  - You must include all desired configuration, as you are completely replacing Goose
-    ///    defaults. For example, if you want Goose clients to store cookies, you will have to
-    ///    include
-    ///    [`.cookie_store(true)`](https://docs.rs/reqwest/*/reqwest/struct.ClientBuilder.html#method.cookie_store).


I believe these last two comments remain relevant, and so should be preserved somewhere in the inline documentation.

The first two comments about test_start and test_stop I believe are no longer true with this PR?

The third comment about building a custom client per GooseUser is no longer possible with this PR?

jeremyandrews · 2021-09-07T07:05:45Z

src/goose.rs

+        self.cookie_store.add_cookie_str(cookie, &self.base_url);
+    }
+
+    /// Default headers can be manually set to each [`GooseUser`](./goose/struct.GooseUser.html).


set on instead of set to

jeremyandrews · 2021-09-07T07:07:15Z

src/goose.rs

+    }
+
+    /// Default headers can be manually set to each [`GooseUser`](./goose/struct.GooseUser.html).
+    /// The headers will automatically been added to each request made by the [`GooseUser`](./goose/struct.GooseUser.html).


be added instead of been added

jeremyandrews · 2021-09-07T07:08:45Z

src/goose.rs

-    /// [`reqwest::cookie::Jar::add_cookie_str`]. Finally, the new cookie jar must be specified as the
-    /// [`reqwest::ClientBuilder::cookie_provider`] for the custom client.
-    ///
+    /// Default headers doesn't overwrite already appended headers.


do not (or don't) instead of doesn't

jeremyandrews · 2021-09-07T07:11:22Z

src/lib.rs

+    /// }
+    /// ```
+    pub fn set_client_builder(mut self, client_builder: ClientBuilder) -> Result<Self, GooseError> {
+        self.client = client_builder.cookie_store(false).build()?;


This seems to make it impossible to enable the cookie_store -- if that's the intent it needs to be documented.

LionsAd · 2021-10-08T19:16:32Z

@finnefloyd Thanks for all your contributions so far!

I share Jeremy's sentiment that I would like to understand the motivation of the change a little bit more, before considering this PR.

netom · 2022-12-20T09:53:36Z

@finnefloyd Thanks for all your contributions so far!

I share Jeremy's sentiment that I would like to understand the motivation of the change a little bit more, before considering this PR.

I used Goose recently, and I needed to create thoudands of users. Starting Goose took about thirty seconds. The tests themselves were not terribly long, so the startup time was significant.

I solved this by monkey-patching goose to use a single reqwest client. (Cookies and headers weren't an issue for me.) This way the statup time disappeared.

I know this is a corner case, but IMHO worth implementing client sharing.

The general argument would be "because it's just more resource efficient".

finnefloyd force-pushed the shared-client branch from 5a3104c to 734d79b Compare September 1, 2021 09:25

finnefloyd added 2 commits September 5, 2021 17:35

Allow shared reqwest client configuration

05be749

Disable cookies in reqwest::Client and handle cookies in GooseUser so…

66e6123

… we can share the client across all running GooseUser

finnefloyd force-pushed the shared-client branch from 734d79b to 66e6123 Compare September 5, 2021 15:56

jeremyandrews self-assigned this Sep 6, 2021

Add add_cookie_str and add_default_header to GooseUser API.

0df6e15

Remove set_client_builder from GooseUser API. Rename set_client to set_client_builder on GooseAttack

jeremyandrews removed their assignment Sep 7, 2021

jeremyandrews requested changes Sep 7, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow shared reqwest client configuration #357

Allow shared reqwest client configuration #357

finnefloyd commented Sep 1, 2021

jeremyandrews commented Sep 2, 2021

jeremyandrews commented Sep 2, 2021

finnefloyd commented Sep 5, 2021

jeremyandrews commented Sep 6, 2021

jeremyandrews commented Sep 6, 2021

jeremyandrews commented Sep 6, 2021

finnefloyd commented Sep 6, 2021

jeremyandrews left a comment

jeremyandrews Sep 7, 2021

jeremyandrews Sep 7, 2021

jeremyandrews Sep 7, 2021

jeremyandrews Sep 7, 2021

jeremyandrews Sep 7, 2021

jeremyandrews Sep 7, 2021

jeremyandrews Sep 7, 2021

jeremyandrews Sep 7, 2021

jeremyandrews Sep 7, 2021

jeremyandrews Sep 7, 2021

LionsAd commented Oct 8, 2021

netom commented Dec 20, 2022

Allow shared reqwest client configuration #357

Are you sure you want to change the base?

Allow shared reqwest client configuration #357

Conversation

finnefloyd commented Sep 1, 2021

jeremyandrews commented Sep 2, 2021

jeremyandrews commented Sep 2, 2021

finnefloyd commented Sep 5, 2021

jeremyandrews commented Sep 6, 2021

jeremyandrews commented Sep 6, 2021

jeremyandrews commented Sep 6, 2021

finnefloyd commented Sep 6, 2021

jeremyandrews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LionsAd commented Oct 8, 2021

netom commented Dec 20, 2022