artilleryio / artillery

The complete load testing platform. Everything you need for production-grade load tests. Serverless & distributed. Load test with Playwright. Load test HTTP APIs, GraphQL, WebSocket, and more. Use any Node.js module.
https://www.artillery.io
Mozilla Public License 2.0
8.02k stars 510 forks source link

Execution threads are multiplicated upon redirection response #1057

Open ambrosmiroslav opened 3 years ago

ambrosmiroslav commented 3 years ago

I have a simple scenario:

config:
  target: "http://localhost:8080"
  phases:
    - duration: 1
      arrivalRate: 1
      name: Warm up
      maxVusers: 1

scenarios:
  - name: "Login"
    flow:
      - get:
          url: "/login"
      - post:
          url: "/login"
          form:
            username: "John Doe"
      - get:
          url: "/list"

which accesses my local HTTP server written in node.js:

var http = require("http");

http.createServer(function (req, res) {
  console.info("Request: " + req.url + ", " + req.method);

  switch(req.url)
  {
    case "/login":
      if (req.method === "GET") {
        res.writeHead(200, { "Content-Type": "text/html" });
        res.write("Login page <form action='/login' method='post'><input name='username'><input type='submit'></form>");
      } else {
        res.writeHead(303, { "Location": "/user" });  
      }
      break;
    case "/user":
      res.write("User page");
      break;
    case "/list":
      res.write("List page");
      break;
    default:
      res.write("Wrong page");
      break;
  }

  res.end(); //end the response
}).listen(8080); //the server object listens on port 8080

This setup tries to simulate user login. The first GET request to /login displays HTML form. Then form submission with POST creates request to /login, the server accepts username and redirects client to /user. And finally user clicks to show some data with /list.

So there should be 4 requests in total (3x GET + 1x POST; the redirect is GET because the "303 See Other" is used). But my HTTP server reports 5 requests - the /list is doubled...

And funny thing - if I repeat the "-post" section in my scenario then I get more and more superfluous /list requests. :-) If I repeat my "-post" section four times I end up with 16 requests to /list... :-)

It seems like a new virtual user is created each time POST + redirect is processed and this new virtual user tries to finish started scenario?

ambrosmiroslav commented 3 years ago

I made some "research" on this problem. It doesn't matter the POST or GET is used - the redirection itself is a problem when automatic redirection following is turned on (which is the default option).

The HTTP requests in artillery are sent by the Got library (https://www.npmjs.com/package/got). The artillery attaches an event listener to the response object to be informed by Got that the request/response was finished, see file core\lib\engine_http.js:

  res.on('end', () => {
    context._successCount++;
    if (!maybeCallback) {
      callback(null, context);
    } else {
      maybeCallback(null, res, body);
    }
  });

What seems to be a problem that Got sends two 'end' events in case of response redirection. One for the initial request (with the response status code 303 in my case). And one for the redirected content (with the response status code 200) - because Got follows the redirection automatically.

So the callback() function is called twice for one scenario step - and since this moment there are two "execution threads" which process the rest of the scenario instead of a single "execution thread". So each "execution thread" which encounters the redirection is duplicated and continues on its own. Thus my sample scenario with four redirects ends up with 2^4 "execution threads" alive...

I can create hotfix for my scenario:

  res.on('end', () => {
    context._successCount++;
    if (code == 303) {
      // do nothing
    } else if (!maybeCallback) {
      callback(null, context);
    } else {
      maybeCallback(null, res, body);
    }
  });

where code is from the closure: let code = res.statusCode; where the response is available.

But to become a general fix it should:

I think I could prepare PR but I'm not sure about the correctness of my solution. ;-)