Skip to content

Commit d66715f

Browse files
takaokoujiGemini
andcommitted
fix: prevent stack overflow in RateLimiter when merging data
- Use arrays to manage resolve/reject callbacks instead of nesting functions - Optimize logging by using log.debug for high-frequency operations - Added periodic statistics reporting for sent and merged items - Added reproduction test case Addresses smalruby/smalruby3-gui#499 Co-Authored-By: Gemini <noreply@google.com>
1 parent 7162f39 commit d66715f

File tree

2 files changed

+94
-15
lines changed

2 files changed

+94
-15
lines changed

src/extensions/scratch3_mesh_v2/rate-limiter.js

Lines changed: 53 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,13 @@ class RateLimiter {
2020
this.enableMerge = options.enableMerge || false;
2121
this.mergeKeyField = options.mergeKeyField || 'key';
2222
this.requestCount = 0;
23+
24+
// Statistics
25+
this.stats = {
26+
totalSent: 0,
27+
totalMerged: 0,
28+
lastReportTime: Date.now()
29+
};
2330
}
2431

2532
/**
@@ -38,7 +45,7 @@ class RateLimiter {
3845
this.queue.push({data, resolve, reject, sendFunction});
3946
}
4047

41-
log.info(`RateLimiter: ${this.enableMerge ? 'Processed' : 'Added'} to queue ` +
48+
log.debug(`RateLimiter: ${this.enableMerge ? 'Processed' : 'Added'} to queue ` +
4249
`(size: ${this.queue.length}, enableMerge: ${this.enableMerge})`);
4350

4451
this.processQueue();
@@ -63,35 +70,64 @@ class RateLimiter {
6370
const existingData = queueItem.data;
6471
const mergedData = this.mergeData(existingData, dataArray);
6572

66-
log.info(`RateLimiter: Merging data - ` +
73+
log.debug(`RateLimiter: Merging data - ` +
6774
`before: ${JSON.stringify(existingData)}, ` +
6875
`after: ${JSON.stringify(mergedData)}`);
6976

7077
queueItem.data = mergedData;
7178

72-
// Chain resolve and reject
73-
const originalResolve = queueItem.resolve;
74-
queueItem.resolve = result => {
75-
originalResolve(result);
76-
resolve(result);
77-
};
79+
// Use arrays to manage resolve/reject callbacks to avoid stack overflow
80+
if (!queueItem.resolvers) {
81+
// Convert existing resolve/reject to arrays
82+
queueItem.resolvers = [queueItem.resolve];
83+
queueItem.rejecters = [queueItem.reject];
84+
85+
// Set new resolve/reject handlers that call all functions in the arrays
86+
queueItem.resolve = result => {
87+
queueItem.resolvers.forEach(r => r(result));
88+
};
89+
queueItem.reject = error => {
90+
queueItem.rejecters.forEach(r => r(error));
91+
};
92+
}
7893

79-
const originalReject = queueItem.reject;
80-
queueItem.reject = error => {
81-
originalReject(error);
82-
reject(error);
83-
};
94+
// Add new callbacks to the arrays
95+
queueItem.resolvers.push(resolve);
96+
queueItem.rejecters.push(reject);
8497

8598
merged = true;
8699
break;
87100
}
88101
}
89102

90-
if (!merged) {
103+
if (merged) {
104+
this.stats.totalMerged++;
105+
this.reportStatsIfNeeded();
106+
} else {
91107
this.queue.push({data: dataArray, resolve, reject, sendFunction});
92108
}
93109
}
94110

111+
/**
112+
* Report statistics periodically.
113+
* @private
114+
*/
115+
reportStatsIfNeeded () {
116+
const now = Date.now();
117+
const elapsed = now - this.stats.lastReportTime;
118+
119+
// Output statistics every 10 seconds
120+
if (elapsed >= 10000) {
121+
log.info(`RateLimiter Stats (last ${(elapsed / 1000).toFixed(1)}s): ` +
122+
`sent=${this.stats.totalSent}, merged=${this.stats.totalMerged}, ` +
123+
`queue=${this.queue.length}`);
124+
125+
this.stats.totalSent = 0;
126+
this.stats.totalMerged = 0;
127+
this.stats.lastReportTime = now;
128+
}
129+
}
130+
95131
/**
96132
* Merge two arrays of data using mergeKeyField.
97133
* @param {Array} existingData - Existing data items.
@@ -161,12 +197,14 @@ class RateLimiter {
161197

162198
const item = this.queue.shift();
163199
this.requestCount++;
164-
log.info(`RateLimiter: Sending request #${this.requestCount} ` +
200+
log.debug(`RateLimiter: Sending request #${this.requestCount} ` +
165201
`(queue remaining: ${this.queue.length})`);
166202

167203
try {
168204
const result = await item.sendFunction(item.data);
169205
this.lastSendTime = Date.now();
206+
this.stats.totalSent++;
207+
this.reportStatsIfNeeded();
170208
item.resolve(result);
171209
} catch (error) {
172210
item.reject(error);
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
const test = require('tap').test;
2+
const RateLimiter = require('../../src/extensions/scratch3_mesh_v2/rate-limiter');
3+
const log = require('../../src/util/log');
4+
5+
// Disable logging for test to avoid timeout
6+
log.debug = () => {};
7+
log.info = () => {};
8+
9+
test('RateLimiter stack overflow reproduction', {timeout: 60000}, async t => {
10+
// maxPerSecond: 1, intervalMs: 250ms, enableMerge: true
11+
const limiter = new RateLimiter(1, 250, {enableMerge: true});
12+
13+
// Immediate sendFunction
14+
const sendFunction = d => Promise.resolve(d);
15+
16+
const promises = [];
17+
18+
// 15000 merges is usually enough to trigger stack overflow in Node.js
19+
const MERGE_COUNT = 15000;
20+
21+
console.log(`Starting ${MERGE_COUNT} merges...`);
22+
23+
for (let i = 0; i < MERGE_COUNT; i++) {
24+
promises.push(limiter.send([{key: 'var1', value: i}], sendFunction));
25+
}
26+
27+
console.log('Finished pushing to queue. Waiting for completion...');
28+
29+
try {
30+
await Promise.all(promises);
31+
t.pass('Completed without stack overflow');
32+
} catch (e) {
33+
if (e.message === 'Maximum call stack size exceeded') {
34+
t.fail(`Stack overflow occurred: ${e.message}`);
35+
} else {
36+
t.fail(`Failed with unexpected error: ${e.message}`);
37+
}
38+
}
39+
40+
t.end();
41+
});

0 commit comments

Comments
 (0)