Skip to content

Commit

Permalink
[minor] Remove async-limiter dependency
Browse files Browse the repository at this point in the history
  • Loading branch information
lpinca committed Nov 29, 2019
1 parent 3293284 commit 950e41a
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 9 deletions.
54 changes: 54 additions & 0 deletions lib/limiter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
'use strict';

This comment has been minimized.

Copy link
@STRML

STRML Nov 30, 2019

Contributor

:) Nice simple adaptation!


const kDone = Symbol('kDone');
const kRun = Symbol('kRun');

/**
* A very simple job queue with adjustable concurrency. Adapted from
* https://github.com/STRML/async-limiter
*/
class Limiter {
/**
* Creates a new `Limiter`.
*
* @param {Number} concurrency The maximum number of jobs allowed to run
* concurrently
*/
constructor(concurrency) {
this[kDone] = () => {
this.pending--;
this[kRun]();
};
this.concurrency = concurrency;

This comment has been minimized.

Copy link
@STRML

STRML Nov 30, 2019

Contributor

One small caveat here, since you're not doing concurrency || Infinity as in the original code: if somebody passes 0, jobs will never run.

This comment has been minimized.

Copy link
@lpinca

lpinca Nov 30, 2019

Author Member

Yeah I thought about this and it could definitely be a breaking change but I think that's what the user expects (or at least what I expect) when using 0?

This comment has been minimized.

Copy link
@STRML

STRML Nov 30, 2019

Contributor

It could be an error - freezing entirely on responses isn't very friendly, especially since the server will appear to come up correctly. There's a small chance some users have interpreted 0 to mean "no limiting"; I've seen concurrency options work that way in other tools. With an error there's a good chance you'd catch this in dev.

This comment has been minimized.

Copy link
@lpinca

lpinca Nov 30, 2019

Author Member

Hmm ok I'll address it here

const concurrency =
this._options.concurrencyLimit !== undefined
? this._options.concurrencyLimit
: 10;
.

This comment has been minimized.

Copy link
@lpinca

lpinca Nov 30, 2019

Author Member

I've ended up using concurrency || Infinity as in the original code as it was easier to test: 6df06d9.

Thanks for the review.

this.jobs = [];
this.pending = 0;
}

/**
* Adds a job to the queue.
*
* @public
*/
add(job) {
this.jobs.push(job);
this[kRun]();
}

/**
* Removes a job from the queue and runs it if possible.
*
* @private
*/
[kRun]() {
if (this.pending === this.concurrency) return;

if (this.jobs.length) {
const job = this.jobs.shift();

this.pending++;
job(this[kDone]);
}
}
}

module.exports = Limiter;
12 changes: 6 additions & 6 deletions lib/permessage-deflate.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
'use strict';

const Limiter = require('async-limiter');
const zlib = require('zlib');

const bufferUtil = require('./buffer-util');
const Limiter = require('./limiter');
const { kStatusCode, NOOP } = require('./constants');

const TRAILER = Buffer.from([0x00, 0x00, 0xff, 0xff]);
Expand Down Expand Up @@ -64,7 +64,7 @@ class PerMessageDeflate {
this._options.concurrencyLimit !== undefined
? this._options.concurrencyLimit
: 10;
zlibLimiter = new Limiter({ concurrency });
zlibLimiter = new Limiter(concurrency);
}
}

Expand Down Expand Up @@ -286,15 +286,15 @@ class PerMessageDeflate {
}

/**
* Decompress data. Concurrency limited by async-limiter.
* Decompress data. Concurrency limited.
*
* @param {Buffer} data Compressed data
* @param {Boolean} fin Specifies whether or not this is the last fragment
* @param {Function} callback Callback
* @public
*/
decompress(data, fin, callback) {
zlibLimiter.push((done) => {
zlibLimiter.add((done) => {
this._decompress(data, fin, (err, result) => {
done();
callback(err, result);
Expand All @@ -303,15 +303,15 @@ class PerMessageDeflate {
}

/**
* Compress data. Concurrency limited by async-limiter.
* Compress data. Concurrency limited.
*
* @param {Buffer} data Data to compress
* @param {Boolean} fin Specifies whether or not this is the last fragment
* @param {Function} callback Callback
* @public
*/
compress(data, fin, callback) {
zlibLimiter.push((done) => {
zlibLimiter.add((done) => {
this._compress(data, fin, (err, result) => {
done();
if (err || result) {
Expand Down
3 changes: 0 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,6 @@
"integration": "npm run lint && mocha --throw-deprecation test/*.integration.js",
"lint": "eslint --ignore-path .gitignore . && prettier --check --ignore-path .gitignore \"**/*.{json,md,yaml,yml}\""
},
"dependencies": {
"async-limiter": "^1.0.0"
},
"peerDependencies": {
"bufferutil": "^4.0.1",
"utf-8-validate": "^5.0.2"
Expand Down
31 changes: 31 additions & 0 deletions test/limiter.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
'use strict';

const assert = require('assert');

const Limiter = require('../lib/limiter');

describe('Limiter', () => {
it('limits the number of jobs allowed to run concurrently', (done) => {
const limiter = new Limiter(1);

limiter.add((callback) => {
setImmediate(() => {
callback();

assert.strictEqual(limiter.jobs.length, 0);
assert.strictEqual(limiter.pending, 1);
});
});

limiter.add((callback) => {
setImmediate(() => {
callback();

assert.strictEqual(limiter.pending, 0);
done();
});
});

assert.strictEqual(limiter.jobs.length, 1);
});
});

0 comments on commit 950e41a

Please sign in to comment.