Skip to content

Commit 91ff683

Browse files
pivstonekinyoklion
andauthored
fix: uncaught DynamoDB exception (#888)
### Description - DynamoDB have rate limit, when we hit read cap, AWS SDK Client will throw an exception, but in the implementation it was using callback. So the callback function will never be called. - So the value will never return to end of client. - An uncaught exception may crash backend application in some cases **Requirements** - [ ] I have added test coverage for new or changed functionality - [x] I have followed the repository's [pull request submission guidelines](../blob/main/CONTRIBUTING.md#submitting-pull-requests) - [x] I have validated my changes against all supported platform versions - the fix is aiming for node server sdk with dynamodb, should be compatible **Related issues** N/A **Describe the solution you've provided** - try catch the error and pass undefined to callback function **Describe alternatives you've considered** - 1. don't use callback, this is a common and typical issue with mixing callback, promise and async/await - 2. to have a error type for callback. usually callback have two parameters: ```ts callback: (value, error) -> void ``` **Additional note** - to cover this kind issue, better to have a integration test, we are using Nock to re-produce this issue in our internal, but LD isn't using such a tool, so we didn't at test in PR. --------- Co-authored-by: Ryan Lamb <[email protected]>
1 parent 245f1a3 commit 91ff683

File tree

3 files changed

+62
-9
lines changed

3 files changed

+62
-9
lines changed

packages/store/node-server-sdk-dynamodb/__tests__/DynamoDBCore.test.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,51 @@ describe('given a store with basic data', () => {
280280
const result = await facade.get(dataKind.features, newFeature.key);
281281
expect(result).toEqual(descriptor);
282282
});
283+
284+
it('handles batchWrite error during init', async () => {
285+
const mockLogger = {
286+
error: jest.fn(),
287+
warn: jest.fn(),
288+
info: jest.fn(),
289+
debug: jest.fn(),
290+
};
291+
const state = new DynamoDBClientState(DEFAULT_CLIENT_OPTIONS);
292+
const errorCore = new DynamoDBCore(DEFAULT_TABLE_NAME, state, mockLogger);
293+
const errorFacade = new AsyncCoreFacade(errorCore);
294+
295+
// Mock batchWrite to throw an error
296+
const error = new Error('Batch write failed');
297+
jest.spyOn(state, 'batchWrite').mockRejectedValueOnce(error);
298+
299+
// Should not throw, but should complete the callback
300+
await expect(errorFacade.init([])).resolves.not.toThrow();
301+
302+
// Verify error was logged
303+
expect(mockLogger.error).toHaveBeenCalledWith(`Error writing to DynamoDB: ${error}`);
304+
});
305+
306+
it('handles get error', async () => {
307+
const mockLogger = {
308+
error: jest.fn(),
309+
warn: jest.fn(),
310+
info: jest.fn(),
311+
debug: jest.fn(),
312+
};
313+
const state = new DynamoDBClientState(DEFAULT_CLIENT_OPTIONS);
314+
const errorCore = new DynamoDBCore(DEFAULT_TABLE_NAME, state, mockLogger);
315+
const errorFacade = new AsyncCoreFacade(errorCore);
316+
317+
// Mock get to throw an error
318+
const error = new Error('Get failed');
319+
jest.spyOn(state, 'get').mockRejectedValueOnce(error);
320+
321+
// Should return undefined when get fails
322+
const result = await errorFacade.get(dataKind.features, 'some-key');
323+
expect(result).toBeUndefined();
324+
325+
// Verify error was logged with correct namespace and key
326+
expect(mockLogger.error).toHaveBeenCalledWith(`Error reading features:some-key: ${error}`);
327+
});
283328
});
284329

285330
it('it can calculate size', () => {

packages/store/node-server-sdk-dynamodb/src/DynamoDBClientState.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,6 @@ export default class DynamoDBClientState {
102102
Key: key,
103103
}),
104104
);
105-
106105
return res.Item;
107106
}
108107

packages/store/node-server-sdk-dynamodb/src/DynamoDBCore.ts

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,11 @@ export default class DynamoDBCore implements interfaces.PersistentDataStore {
167167
// Always write the initialized token when we initialize.
168168
ops.push({ PutRequest: { Item: this._initializedToken() } });
169169

170-
await this._state.batchWrite(this._tableName, ops);
170+
try {
171+
await this._state.batchWrite(this._tableName, ops);
172+
} catch (error) {
173+
this._logger?.error(`Error writing to DynamoDB: ${error}`);
174+
}
171175
callback();
172176
}
173177

@@ -176,13 +180,18 @@ export default class DynamoDBCore implements interfaces.PersistentDataStore {
176180
key: string,
177181
callback: (descriptor: interfaces.SerializedItemDescriptor | undefined) => void,
178182
) {
179-
const read = await this._state.get(this._tableName, {
180-
namespace: stringValue(this._state.prefixedKey(kind.namespace)),
181-
key: stringValue(key),
182-
});
183-
if (read) {
184-
callback(this._unmarshalItem(read));
185-
} else {
183+
try {
184+
const read = await this._state.get(this._tableName, {
185+
namespace: stringValue(this._state.prefixedKey(kind.namespace)),
186+
key: stringValue(key),
187+
});
188+
if (read) {
189+
callback(this._unmarshalItem(read));
190+
} else {
191+
callback(undefined);
192+
}
193+
} catch (error) {
194+
this._logger?.error(`Error reading ${kind.namespace}:${key}: ${error}`);
186195
callback(undefined);
187196
}
188197
}

0 commit comments

Comments
 (0)