fix: whenCurrentJobsFinished shouldn't initialize bclient

[gabegorelick]

Fixes #1346

[manast]

Couldn't you store this information in a separate data structure? furthermore, what other queue types would benefit from storing this data?

[gabegorelick]

Couldn't you store this information in a separate data structure?

Not sure I follow. Do you mean you want a dedicated object on the queue to save this? Something like

queue._clientStatuses = {}; // in Queue constructor

queue._clientStatuses[type] = 'initialized'; // after initialization

Or are you thinking there's a more complex state machine that should be used?

what other queue types would benefit from storing this data?

Do you mean Redis connection types? There may be instances where client and eclient are also initialized unnecessarily, but I haven't tracked them down yet. That's also blocked by #1344 which means that client is always initialized.

[gabegorelick]

@manast Have you had a chance to look at this?

[manast]

yes, sorry. I meant a separate data structure.

[gabegorelick]

The failing test appears to be an existing race condition:

bull/test/test_connection.js

Lines 82 to 117 in c0ee6be

	it('should not close external connections', () => {
	const client = new redis();
	const subscriber = new redis();
	const opts = {
	createClient(type) {
	switch (type) {
	case 'client':
	return client;
	case 'subscriber':
	return subscriber;
	default:
	return new redis();
	}
	}
	};
	const testQueue = utils.buildQueue('external connections', opts);
	return testQueue
	.isReady()
	.then(() => {
	return testQueue.add({ foo: 'bar' });
	})
	.then(() => {
	expect(testQueue.client).to.be.eql(client);
	expect(testQueue.eclient).to.be.eql(subscriber);
	return testQueue.close();
	})
	.then(() => {
	expect(client.status).to.be.eql('ready');
	expect(subscriber.status).to.be.eql('ready');
	return Promise.all([client.quit(), subscriber.quit()]);
	});
	});

isReady only waits for client to be ready, not the subscriber client. subscriber client is initialized in the Queue constructor when we call this.on('error'), so it's a race condition whether it's ready or not by the time the test gets to that assertion. I don't see anything in my PR that would change this.

[coveralls]

Coverage increased (+0.08%) to 93.027% when pulling 2f9b580 on gabegorelick:pause-bclient into c0ee6be on OptimalBits:develop.

[coveralls]

Coverage increased (+0.08%) to 93.027% when pulling 2f9b580 on gabegorelick:pause-bclient into c0ee6be on OptimalBits:develop.

[mgreenw]

This is awesome, thanks for fixing this @gabegorelick!