Skip to content

Commit 23da59e

Browse files
laurenzlonglaurenzlong
authored
Change release function to only decrement counter after waiting 2 min. (#31)
1 parent dc9e340 commit 23da59e

File tree

2 files changed

+124
-90
lines changed

2 files changed

+124
-90
lines changed

‎spec/apps.spec.ts‎

Lines changed: 112 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -64,102 +64,142 @@ describe('apps', () => {
6464
expect(userApp).to.equal(userAppAgain);
6565
});
6666

67-
it('should retain/release ref counters appropriately without auth', function() {
68-
apps.retain({});
69-
expect(apps['_refCounter']).to.deep.equal({
70-
__admin__: 1,
71-
__noauth__: 1,
72-
});
73-
apps.release({});
74-
expect(apps['_refCounter']).to.deep.equal({
75-
__admin__: 0,
76-
__noauth__: 0,
77-
});
78-
});
79-
80-
it('should retain/release ref counters appropriately with admin auth', function() {
81-
apps.retain({auth: {admin: true}});
82-
expect(apps['_refCounter']).to.deep.equal({
83-
__admin__: 2,
84-
});
85-
apps.release({auth: {admin: true}});
86-
expect(apps['_refCounter']).to.deep.equal({
87-
__admin__: 0,
88-
});
89-
});
90-
91-
it('should retain/release ref counters appropriately with user auth', function() {
92-
const payload = {auth: {admin: false, variable: claims}};
93-
const userAppName = apps._appName(payload.auth);
94-
apps.retain(payload);
95-
expect(apps['_refCounter']).to.deep.equal({
96-
__admin__: 1,
97-
[userAppName]: 1,
98-
});
99-
apps.release(payload);
100-
expect(apps['_refCounter']).to.deep.equal({
101-
__admin__: 0,
102-
[userAppName]: 0,
103-
});
104-
});
105-
106-
describe('#_waitToDestroyApp', () => {
67+
describe('retain/release', () => {
10768
let clock;
108-
let noauthApp;
109-
let deleteNoauth;
11069

11170
beforeEach(() => {
11271
clock = sinon.useFakeTimers();
113-
noauthApp = apps.forMode({ admin: false });
114-
deleteNoauth = noauthApp.delete.bind(noauthApp);
115-
sinon.spy(noauthApp, 'delete');
11672
});
11773

11874
afterEach(() => {
11975
clock.restore();
120-
noauthApp.delete.restore();
12176
});
12277

123-
it('should delete app after 2 minutes and not earlier', function() {
124-
apps['_refCounter'] = { '__noauth__': 0 };
125-
apps._waitToDestroyApp('__noauth__');
126-
clock.tick(appsNamespace.garbageCollectionInterval - 1);
127-
expect(noauthApp.delete.called).to.be.false;
128-
clock.tick(1);
129-
130-
// Technically the delete happens on the next tick *after* 2 min
78+
it('should retain/release ref counters appropriately without auth', function() {
79+
apps.retain({});
80+
expect(apps['_refCounter']).to.deep.equal({
81+
__admin__: 1,
82+
__noauth__: 1,
83+
});
84+
apps.release({});
85+
clock.tick(appsNamespace.garbageCollectionInterval);
13186
return Promise.resolve().then(() => {
132-
expect(noauthApp.delete.called).to.be.true;
87+
expect(apps['_refCounter']).to.deep.equal({
88+
__admin__: 0,
89+
__noauth__: 0,
90+
});
13391
});
13492
});
13593

136-
it('should exit right away if app was already deleted', function() {
137-
return deleteNoauth().then(() => {
138-
let spy = sinon.spy(appsNamespace, 'delay');
139-
apps._waitToDestroyApp('__noauth__');
140-
spy.restore();
141-
expect(spy.called).to.be.false;
94+
it('should retain/release ref counters appropriately with admin auth', function() {
95+
apps.retain({auth: {admin: true}});
96+
expect(apps['_refCounter']).to.deep.equal({
97+
__admin__: 2,
98+
});
99+
apps.release({auth: {admin: true}});
100+
clock.tick(appsNamespace.garbageCollectionInterval);
101+
return Promise.resolve().then(() => {
102+
expect(apps['_refCounter']).to.deep.equal({
103+
__admin__: 0,
104+
});
142105
});
143106
});
144107

145-
it('should not delete app if it gets deleted while function is waiting', function() {
146-
apps._waitToDestroyApp('__noauth__');
147-
deleteNoauth();
108+
it('should retain/release ref counters appropriately with user auth', function() {
109+
const payload = {auth: {admin: false, variable: claims}};
110+
const userAppName = apps._appName(payload.auth);
111+
apps.retain(payload);
112+
expect(apps['_refCounter']).to.deep.equal({
113+
__admin__: 1,
114+
[userAppName]: 1,
115+
});
116+
apps.release(payload);
148117
clock.tick(appsNamespace.garbageCollectionInterval);
149118
return Promise.resolve().then(() => {
150-
expect(noauthApp.delete.called).to.be.false;
119+
expect(apps['_refCounter']).to.deep.equal({
120+
__admin__: 0,
121+
[userAppName]: 0,
122+
});
151123
});
152124
});
153125

154-
it('should not delete app if ref count rises above 0', function() {
155-
apps['_refCounter'] = {
156-
'__admin__': 0,
157-
'__noauth__': 1,
158-
};
159-
apps._waitToDestroyApp('__noauth__');
126+
it('should only decrement counter after garbageCollectionInterval is up', function() {
127+
apps.retain({});
128+
apps.release({});
129+
clock.tick(appsNamespace.garbageCollectionInterval / 2);
130+
expect(apps['_refCounter']).to.deep.equal({
131+
__admin__: 1,
132+
__noauth__: 1,
133+
});
134+
clock.tick(appsNamespace.garbageCollectionInterval / 2);
135+
return Promise.resolve().then(() => {
136+
expect(apps['_refCounter']).to.deep.equal({
137+
__admin__: 0,
138+
__noauth__: 0,
139+
});
140+
});
141+
});
142+
143+
it('should call _destroyApp if app no longer used', function() {
144+
let spy = sinon.spy(apps, '_destroyApp');
145+
apps.retain({});
146+
apps.release({});
160147
clock.tick(appsNamespace.garbageCollectionInterval);
161148
return Promise.resolve().then(() => {
162-
expect(noauthApp.delete.called).to.be.false;
149+
expect(spy.called).to.be.true;
150+
});
151+
});
152+
153+
it('should not call _destroyApp if app used again while waiting for release', function() {
154+
let spy = sinon.spy(apps, '_destroyApp');
155+
apps.retain({});
156+
apps.release({});
157+
clock.tick(appsNamespace.garbageCollectionInterval / 2);
158+
apps.retain({});
159+
clock.tick(appsNamespace.garbageCollectionInterval / 2);
160+
return Promise.resolve().then(() => {
161+
expect(spy.called).to.be.false;
162+
});
163+
});
164+
165+
it('should increment ref counter for each subsequent retain', function() {
166+
apps.retain({});
167+
expect(apps['_refCounter']).to.deep.equal({
168+
__admin__: 1,
169+
__noauth__: 1,
170+
});
171+
apps.retain({});
172+
expect(apps['_refCounter']).to.deep.equal({
173+
__admin__: 2,
174+
__noauth__: 2,
175+
});
176+
apps.retain({});
177+
expect(apps['_refCounter']).to.deep.equal({
178+
__admin__: 3,
179+
__noauth__: 3,
180+
});
181+
});
182+
183+
it('should work with staggering sets of retain/release', function() {
184+
apps.retain({});
185+
apps.release({});
186+
clock.tick(appsNamespace.garbageCollectionInterval / 2);
187+
apps.retain({});
188+
apps.release({});
189+
clock.tick(appsNamespace.garbageCollectionInterval / 2);
190+
return Promise.resolve().then(() => {
191+
// Counters are still 1 due second set of retain/release
192+
expect(apps['_refCounter']).to.deep.equal({
193+
__admin__: 1,
194+
__noauth__: 1,
195+
});
196+
clock.tick(appsNamespace.garbageCollectionInterval / 2);
197+
}).then(() => {
198+
// It's now been a full interval since the second set of retain/release
199+
expect(apps['_refCounter']).to.deep.equal({
200+
__admin__: 0,
201+
__noauth__: 0,
202+
});
163203
});
164204
});
165205
});

‎src/apps.ts‎

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -91,19 +91,11 @@ export namespace apps {
9191
}
9292
}
9393

94-
_waitToDestroyApp(appName: string) {
94+
_destroyApp(appName: string) {
9595
if (!this._appAlive(appName)) {
96-
return Promise.resolve();
96+
return;
9797
}
98-
return delay(120000).then(() => {
99-
if (!this._appAlive(appName)) {
100-
return;
101-
}
102-
if (_.get(this._refCounter, appName) === 0) {
103-
delete this._refCounter[appName];
104-
return firebase.app(appName).delete().catch(_.noop);
105-
}
106-
});
98+
firebase.app(appName).delete().catch(_.noop);
10799
}
108100

109101
retain(payload) {
@@ -113,7 +105,7 @@ export namespace apps {
113105
};
114106
// Increment counter for admin because function might use event.data.adminRef
115107
_.update(this._refCounter, '__admin__', increment);
116-
// Increment counter for according to auth type because function might use event.data.ref
108+
// Increment counter according to auth type because function might use event.data.ref
117109
_.update(this._refCounter, this._appName(auth), increment);
118110
}
119111

@@ -122,12 +114,14 @@ export namespace apps {
122114
let decrement = n => {
123115
return n - 1;
124116
};
125-
_.update(this._refCounter, '__admin__', decrement);
126-
_.update(this._refCounter, this._appName(auth), decrement);
127-
_.forEach(this._refCounter, (count, key) => {
128-
if (count === 0) {
129-
this._waitToDestroyApp(key);
130-
}
117+
return delay(garbageCollectionInterval).then(() => {
118+
_.update(this._refCounter, '__admin__', decrement);
119+
_.update(this._refCounter, this._appName(auth), decrement);
120+
_.forEach(this._refCounter, (count, key) => {
121+
if (count <= 0) {
122+
this._destroyApp(key);
123+
}
124+
});
131125
});
132126
}
133127

0 commit comments

Comments
 (0)