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

Mangling of binary response bodies #1044

Closed
na-- opened this issue Jun 7, 2019 · 0 comments Β· Fixed by #1047
Closed

Mangling of binary response bodies #1044

na-- opened this issue Jun 7, 2019 · 0 comments Β· Fixed by #1047
Assignees

Comments

@na--
Copy link
Member

na-- commented Jun 7, 2019

While benchmarking my fixes to #1041, I found another issue that has been lurking in k6 for a similar amount of time now πŸ€¦β€β™‚οΈ Specifically, it has to do with the buffer pools that are used for storing response bodies when the specified responseType is binary: https://github.com/loadimpact/k6/blob/607b63c4400c8b6852c7f51a0a6151f0bb8ec6b7/lib/netext/httpext/request.go#L367-L379

Notice how we get a bytes.Buffer from a the buffer pool BPool, reset it, read the response body into it, save the result as binary or a string to be exported to the JS script and continue. The buffer is then returned back to the pool (defer state.BPool.Put(buf)).

The problem with that piece of code is that buf.Bytes() doesn't copy the data - it references the same underlying byte slice that the buffer uses. So, if a buffer gets reused before the current iteration has ended (which can easily happen, especially with http.batch()), it can still have references pointing to it. So, the old response body may get mangled... buf.String() doesn't have the same problem, since Go strings are immutable: https://play.golang.org/p/RLoDrXnVRiK

Here's a script demonstrating the issue:

import http from 'k6/http';

const testServer = "https://httpbin.test.loadimpact.com"
const batchSize = 150;

function bin2String(array) {
    var result = "";
    for (var i = 0; i < array.length; i++) {
        result += String.fromCharCode(array[i]);
    }
    return result;
}

export default function () {
    let requests = [];
    for (let i = 0; i < batchSize; i++) {
        requests.push(["GET", `${testServer}/get?req=${i}`, null, { responseType: "binary" }]);
    }

    let responses = http.batch(requests);

    for (let i = 0; i < batchSize; i++) {
        try {
            let reqNumber = responses[i].json().args.req;
            if (i != reqNumber) {
                console.log(`[${__VU}:${__ITER}:${i}] got ${reqNumber}, expected ${i}...`)
            }
        } catch (err) {
            console.log(`[${__VU}:${__ITER}:${i}] Error: \n${err}\n\nBody: \n` + bin2String(responses[i].body));
        }
    }
};

And while the buffer pools have been present since k6 v0.17.2 (added with commit b002f2c), the actual issue only occurred with the addition of binary response bodies in k6 v0.23.0.

@na-- na-- self-assigned this Jun 7, 2019
na-- added a commit that referenced this issue Jun 10, 2019
This should fix #1041, #1044, #925, and a potential minor data race! Hopefully, without introducing new bugs or preformance regressions...
na-- added a commit that referenced this issue Jun 13, 2019
Fix a bunch of HTTP measurement and handling issues

This should fix #1041, #1044, #925, and a potential minor data race! Hopefully, without introducing new bugs or performance regressions...
srguglielmo pushed a commit to srguglielmo/k6 that referenced this issue Nov 3, 2019
Fix a bunch of HTTP measurement and handling issues

This should fix grafana#1041, grafana#1044, grafana#925, and a potential minor data race! Hopefully, without introducing new bugs or performance regressions...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant