Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cookie manager doesn't represent all cookies #32

Closed
syampol opened this issue Sep 21, 2022 · 8 comments
Closed

Cookie manager doesn't represent all cookies #32

syampol opened this issue Sep 21, 2022 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@syampol
Copy link

syampol commented Sep 21, 2022

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

According to log:
image
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
Copy link
Contributor

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?

  1. What version of the plugin are you using
  2. What version of JMeter are you using
  3. 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
Copy link
Author

syampol commented Sep 28, 2022

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
Copy link
Contributor

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 🚀

@RicardoPoleo RicardoPoleo added the bug Something isn't working label Sep 28, 2022
@syampol13
Copy link

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
Copy link
Contributor

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.

@3dgiordano 3dgiordano self-assigned this Feb 25, 2023
@SurenSritharan
Copy link

@3dgiordano I've got a fix for saving multiple cookies. Please see: SurenSritharan@58669d2

@3dgiordano
Copy link
Contributor

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants