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

fix default option issue #125

Merged
merged 1 commit into from
Mar 4, 2018
Merged

fix default option issue #125

merged 1 commit into from
Mar 4, 2018

Conversation

WLBF
Copy link
Contributor

@WLBF WLBF commented Mar 2, 2018

I get something wrong when I run two autocannon benchmark at same time. However if I run these benchmarks separately it works fine.
example:

const autocannon = require('autocannon');
const util = require('util');

const autocannonFunc = util.promisify(autocannon);

function setupClient {
    ...
}

async function login() {
  try {
    return await autocannonFunc({
      url: 'http://192.168.0.245:10000/login',
      connections: 15,
      pipelining: 1,
      amount: 15,
      bailout: 1,
      method: 'POST',
      setupClient,
    });
  } catch(err) {
      ...
  }
}

async function register() {
  try {
    return await autocannonFunc({
      url: 'http://192.168.0.245:8888/register',
      connections: 1,
      pipelining: 10,
      amount: 20,
      bailout: 1,
      method: 'POST',
      headers: {
        'Content-Type': 'application/json',
      },
      body: JSON.stringify(body),
    });
  } catch(err) {
      ...
  }
}

Promise.all([login(), register()])

run above code , second autocannon instance will return abnormal 5xx code . I figure out that first calling of run() function change the requests field of defaultOptions object, cause second calling failed. So I think it should be fine to move defaultOptions into function run() block to avoid the side effect.

@mcollina
Copy link
Owner

mcollina commented Mar 2, 2018

I think the proper fix is to switch from xtend to Object.assign in https://github.com/mcollina/autocannon/pull/125/files#diff-de78b50723ff596d7bd35665f36a6cbfR73.

Can you also add a unit test for this?

@WLBF
Copy link
Contributor Author

WLBF commented Mar 2, 2018

sure, I am working on it.

@WLBF
Copy link
Contributor Author

WLBF commented Mar 3, 2018

I think Object.assign only copy first layer of a object. Here we need do full deep copy of defaultOptions object to avoid modification come from run() function. I also adjust the position of defaultOptions to add a unit test.

Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! a couple of nits!

package.json Outdated
"color-support": "^1.1.1",
"deep-equal": "^1.0.1",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no need for this dependency, tap can do it for us.

test/run.test.js Outdated
duration: 2
}, function (err, result) {
t.error(err)
t.ok(deepEqual(defaultOptions, origin), 'calling run function does not modify default options')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use t.deepEqual here.

@WLBF
Copy link
Contributor Author

WLBF commented Mar 4, 2018

ok

Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina mcollina merged commit 3cb9aa1 into mcollina:master Mar 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants