Cookie manager doesn't represent all cookies

syampol commented 1 year ago

Cookies obtained from 1st request: Only one of them was set within the next request

According to log: all four parameters were compressed with Hpack for the 2nd request.

But 1st of all this is not visible in UI. And 2nd - I still getting set-cookie on 2nd request for two of them

RicardoPoleo commented 1 year ago

Hello @syampol,

Thanks for taking the time to report this behaviour, it's a shame you are experiencing it.

In order to give a more accurate report, narrow down the issue and give you a faster solution, please, could you provide the following information?

What version of the plugin are you using
What version of JMeter are you using
Could you provide a JMX or a way to replicate this behaviour?

After this, we are going to check it out in our end and come back to you as we detect and find a solution.

Once again, thanks for taking the time,

Regards

syampol commented 1 year ago

hi @RicardoPoleo issue_32.zip tp attached.

jmeter 5.5 plugin - 2.0 from plugin manager (same for 2.0 build from commit:f781d43 in master). also tried 2.0.1 build from latest master, but failed with a 2sec timeout on a very 1st request (which is executed longer). haven't made out where this 2sec cam from so roll back to 2.0

discover several problems with the plugin. trying describe:

Jmeter & jetty client both have their own cookie manager, and those seems not synced (especially if you add/modify some cookie in Jmeter CM via groovy for example) each of those CM send their own cookies to the server. so the server gets duplicated records for most of params. for us we add custom filtering to remove from jetty' CM what already exists in a jmeter' CM. also jetty' CM needs to be reset on a new iteration in accordance to the Jmeter' CM.
cookies managed via jetty' CM only are not visible in the request in Jmeter UI
similar behavior with headers - jetty' http client adds it's own headers (user-agent & accept-encoding) even if such are defined within the Jmeter' Header Manager
in attached TP - you may see a new connection and new jetty http client (and 10 additional threads 'httpClient@...') are created on every new iteration within any controller. this also results in a new jetty' CM. but Jmeter CM remain the same. i believe this is due to LoopIterationListener.iterationStart nature. it occurs on any loop iteration and not only for the TG iteration. on the other hand ThreadListener: threadStarteded/threadFinished are triggered only on the test start/finish. not on the thread iteration start/finish. so have no clue how to properly solve this to make a new client/connection created on the thread iteration start.. for us we just pass LoopIterationListener.iterationStart and don't recreate a client here. not fully real behavior, but more less acceptable.
every jetty http client opens 10 additional threads (on httpClient.start()). from what i've seen via profiler most of the time thos threads are idle, so i don't fully understand why would we need 10 of them per each VU. this creates a huge overhead on both system and JVM with increasing VU numbers. for example having 4K VUs running from a single VM - this will require ~44K opened threads. with a Xss512K - it will require 22Gb ram only for creating those threads. switching between those threads is also quite expensive. and intensive thread creation/finalization has additional impact from the system perspective. so it would be very useful to control that jetty's thread pool somehow and keep it as low as possible for better performance some details on this in issue#24

p.s. tried to create a branch with our custom workarounds for cookie handling. but failed to make push due to some permission restrictions. wasn't able to make it out..

RicardoPoleo commented 1 year ago

Hello @syampol,

We need to investigate that behavior for the 10 additional threads, because even though every VU in Jmeter, http2 connection or static resource opens a thread (or at least that's we have intended for it), the fact that it opens those 10 threads because of reasons, and don't use them, seems pretty weird. I apologize for it.

Also, regarding those changes for the CM: Did you make a public fork here in GitHub? I checked the repository's forks and didn't see it. If you haven't, could you give it a try and make a PR? We would review them and also mention you on the contributions for the release.

In the meanwhile, we will review this and the issue #24 on our end, to try to fix it.

Anyway, thank you very much for the extensive and detailed discoveries 🚀

syampol13 commented 1 year ago

Hi @RicardoPoleo

Regarding 10 additional threads - I didn't say those are not used. I said those looks mainly idle while watching with profile. Threads are created with the jetty httpClient.start(). I believe 10 is some default setting for the used thread pool. Need to investigate if this is configurable and if diminishing thread count may impact functionality itself. As I understand those additional threads are responsible for the communication and data processing (send, recv, pack, unpack etc.)

As for public fork - as I said I haven't succeed in this. Always getting permission denied error and can't solve it (( As for code change it's basically iterating jetty' CM in the HTTP2JettyClient.buildCookies and removing duplicates. It's not a full and mature fix, it's just a workaround that is suitable for our current needs (in combination with disabling the closeConnections in the HTTP2Sampler.iterationStart. This to not re-create a connecting and a jetty client on each new iteration)

Also regarding thread amount - what one of our DEV did is create a jetty http client with a configurable QueuedThreadPool thread pool. I haven't observed any unexpected issues running our test with such TP, configured with 3/4 as min/max TP size (vs 10/100 default settings). But still can't 100% sure tell that TP size can be shrunk with no future impact. It's just an observation from only one custom run...

3dgiordano commented 1 year ago

Hi @syampol13

We have made improvements to the http2 client. You can download the latest release or the latest alpha version and let us know if the reported issues are still occurring.

The alpha version https://github.com/Blazemeter/jmeter-http2-plugin/releases/tag/v2.0.3-alpha The latest release 2.0.2 https://github.com/Blazemeter/jmeter-http2-plugin/releases/tag/v2.0.2

Let us know if it resolves what was reported in this issue.

PS: Related to cookies, If you have a fork where you made the fixes to the cookie manager, you can share the link with us and we will be able to see those suggestions.

SurenSritharan commented 1 year ago

@3dgiordano I've got a fix for saving multiple cookies. Please see: https://github.com/SurenSritharan/jmeter-http2-plugin/commit/58669d26557ca6cc68296a89bbc6479eb2334373

3dgiordano commented 1 year ago

Hi @SurenSritharan

Thank you for your contribution, indeed there was a bug there. We fixed it and built in unit tests so we don't have a regression. The fix will be available in the next version release.

But you can try all the improvements that have been worked on lately in the alpha.2 version. It is available as a pre-release. https://github.com/Blazemeter/jmeter-http2-plugin/releases/tag/v2.0.3-alpha.2

3dgiordano commented 8 months ago

Hi @SurenSritharan

The fix was released on the version 2.0.3/2.0.4

I'm going to close this issue assuming it was resolved.

If not, you can request reopening again, informing what the problem was detected with respect to what was reported in this issue.

Thank you.

Blazemeter / jmeter-http2-plugin

Cookie manager doesn't represent all cookies #32