Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion docs/features/batch.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ Processing batches from SQS works in three stages:
By default, the batch processor will process messages in parallel, which does not guarantee the order of processing. If you need to process messages in order, set the [`processInParallel` option to `false`](#sequential-processing), or use [`SqsFifoPartialProcessor` for SQS FIFO queues](#fifo-queues).

!!! note
If you're migrating from `BatchProcessorSync` to `BatchProcessor`, note that `processPartialResponse` is async and returns a promise.
If you're migrating from `BatchProcessorSync` to `BatchProcessor`, note that `processPartialResponse` is async and returns a promise. The synchronous processors have no way to await a record handler, so they throw `AsyncHandlerNotSupportedError` when the handler returns one.

=== "index.ts"

Expand Down Expand Up @@ -125,6 +125,9 @@ By default, we will stop processing at the first failure and mark unprocessed me

Enable the `skipGroupOnError` option for seamless processing of messages from various group IDs. This setup ensures that messages from a failed group ID are sent back to SQS, enabling uninterrupted processing of messages from the subsequent group ID.

!!! note
`SqsFifoPartialProcessor` is synchronous and throws `AsyncHandlerNotSupportedError` when the record handler returns a promise. Use `SqsFifoPartialProcessorAsync` together with `processPartialResponse` for asynchronous record handlers.

=== "index.ts"

```typescript hl_lines="1-4 8 20"
Expand Down
30 changes: 26 additions & 4 deletions packages/batch/src/BatchProcessorSync.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import { BasePartialBatchProcessor } from './BasePartialBatchProcessor.js';
import { BatchProcessingError, toError } from './errors.js';
import {
AsyncHandlerNotSupportedError,
BatchProcessingError,
toError,
} from './errors.js';
import type { BaseRecord, FailureResponse, SuccessResponse } from './types.js';

/**
Expand Down Expand Up @@ -111,15 +115,33 @@ import type { BaseRecord, FailureResponse, SuccessResponse } from './types.js';
public processRecordSync(
record: BaseRecord
): SuccessResponse | FailureResponse {
let result: unknown;
try {
const data = this.toBatchType(record, this.eventType);
const result = this.handler(data, this.options?.context);

return this.successHandler(record, result);
result = this.handler(data, this.options?.context);
} catch (error) {
return this.failureHandler(record, toError(error));
}

if (isThenable(result)) {
// The promise is abandoned here, so take ownership of its rejection first:
// otherwise a rejecting handler also surfaces as an unhandled rejection, which
// the Lambda runtime reports instead of the error thrown below.
result.then(undefined, () => undefined);
Comment on lines +127 to +130

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guard itself makes sense, and I agree the rejection has to be claimed before the throw or the Lambda runtime reports Runtime.UnhandledPromiseRejection instead of this error. Two things about how it's claimed though.

The no-op handler discards the customer's actual failure. By this point the async handler has already been invoked for the first record, so if it rejects, that rejection is the only evidence of what went wrong inside their code. With this line they see "wrong processor" and nothing else. I'd rather log the reason so the guard doesn't hide a second bug:

Suggested change
// The promise is abandoned here, so take ownership of its rejection first:
// otherwise a rejecting handler also surfaces as an unhandled rejection, which
// the Lambda runtime reports instead of the error thrown below.
result.then(undefined, () => undefined);
// The promise cannot be awaited here. Claim its rejection so a failing handler
// doesn't also surface as Runtime.UnhandledPromiseRejection and mask the error
// thrown below; log the reason so the handler's own failure is not lost.
result.then(undefined, (reason) => {
console.error(
'Record handler returned a promise to a synchronous batch processor and later rejected',
reason
);
});

Separately, the comment says the code "takes ownership" of the rejection, which reads as if it's handled. It's suppressed, and the comment is clearer if it says that. The suggestion above rewords it.

One more nit, not on this line: since the handler has already run once when this throws, it'd help if AsyncHandlerNotSupportedError said so. Something like "The record handler was invoked for the first record and returned a promise, but this batch processor is synchronous and cannot await it..." tells the reader that side effects may already be in flight.


throw new AsyncHandlerNotSupportedError();
}

return this.successHandler(record, result);
}
}

/**
* Type guard to detect a thenable (promise-like) value returned by a record handler.
*
* @param value - The value returned by the record handler
*/
const isThenable = (value: unknown): value is PromiseLike<unknown> =>
typeof (value as PromiseLike<unknown> | undefined)?.then === 'function';

export { BatchProcessorSync };
7 changes: 3 additions & 4 deletions packages/batch/src/SqsFifoPartialProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,14 @@ import type {
* @example
* ```typescript
* import {
* BatchProcessor,
* EventType,
* processPartialResponseSync,
* SqsFifoPartialProcessor,
* } from '@aws-lambda-powertools/batch';
* import type { SQSRecord, SQSHandler } from 'aws-lambda';
*
* const processor = new BatchProcessor(EventType.SQS);
* const processor = new SqsFifoPartialProcessor();
*
* const recordHandler = async (record: SQSRecord): Promise<void> => {
* const recordHandler = (record: SQSRecord): void => {
* const payload = JSON.parse(record.body);
* };
*
Expand Down
14 changes: 14 additions & 0 deletions packages/batch/src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,19 @@ class BatchProcessingError extends Error {
}
}

/**
* Error thrown by the Batch Processing utility when a record handler returns a promise
* to a synchronous batch processor, which has no way to await it.
*/
class AsyncHandlerNotSupportedError extends BatchProcessingError {
public constructor() {
super(
'The record handler returned a promise, but this batch processor is synchronous and cannot await it. Use BatchProcessor together with processPartialResponse(), or SqsFifoPartialProcessorAsync for FIFO queues.'
);
this.name = 'AsyncHandlerNotSupportedError';
}
}

/**
* Error thrown by the Batch Processing utility when all batch records failed to be processed
*/
Expand Down Expand Up @@ -111,6 +124,7 @@ const toError = (value: unknown): Error => {
};

export {
AsyncHandlerNotSupportedError,
BatchProcessingError,
FullBatchFailureError,
ParsingError,
Expand Down
1 change: 1 addition & 0 deletions packages/batch/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ export { BatchProcessor } from './BatchProcessor.js';
export { BatchProcessorSync } from './BatchProcessorSync.js';
export { EventType } from './constants.js';
export {
AsyncHandlerNotSupportedError,
BatchProcessingError,
FullBatchFailureError,
ParsingError,
Expand Down
10 changes: 5 additions & 5 deletions packages/batch/src/processPartialResponseSync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@ import type {
* @example
* ```typescript
* import {
* BatchProcessor,
* BatchProcessorSync,
* EventType,
* processPartialResponseSync,
* } from '@aws-lambda-powertools/batch';
* import type { SQSRecord, SQSHandler } from 'aws-lambda';
*
* const processor = new BatchProcessor(EventType.SQS);
* const processor = new BatchProcessorSync(EventType.SQS);
*
* const recordHandler = async (record: SQSRecord): Promise<void> => {
* const recordHandler = (record: SQSRecord): void => {
* const payload = JSON.parse(record.body);
* };
*
Expand All @@ -59,7 +59,7 @@ import type {
*
* const processor = new SqsFifoPartialProcessor();
*
* const recordHandler = async (record: SQSRecord): Promise<void> => {
* const recordHandler = (record: SQSRecord): void => {
* const payload = JSON.parse(record.body);
* };
*
Expand All @@ -83,7 +83,7 @@ import type {
*
* const processor = new SqsFifoPartialProcessor();
*
* const recordHandler = async (record: SQSRecord): Promise<void> => {
* const recordHandler = (record: SQSRecord): void => {
* const payload = JSON.parse(record.body);
* };
*
Expand Down
67 changes: 67 additions & 0 deletions packages/batch/tests/unit/BatchProcessorSync.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import context from '@aws-lambda-powertools/testing-utils/context';
import type { SQSRecord } from 'aws-lambda';
import { describe, expect, it } from 'vitest';
import {
AsyncHandlerNotSupportedError,
BatchProcessorSync,
EventType,
processPartialResponseSync,
SqsFifoPartialProcessor,
} from '../../src/index.js';
import { sqsRecordFactory } from '../helpers/factories.js';

describe('Class: BatchProcessorSync', () => {
const asyncRecordHandler = async (record: SQSRecord): Promise<string> =>
record.body;

const rejectingRecordHandler = async (
_record: SQSRecord
): Promise<string> => {
throw new Error('failed');
};

it('throws when the record handler returns a promise', () => {
// Prepare
const records = [sqsRecordFactory('success'), sqsRecordFactory('success')];
const batch = { Records: records };
const processor = new BatchProcessorSync(EventType.SQS);

// Act & Assess
expect(() =>
processPartialResponseSync(batch, asyncRecordHandler, processor, {
context,
})
).toThrow(AsyncHandlerNotSupportedError);
expect(processor.successMessages).toHaveLength(0);
});

it('leaves no unhandled rejection behind when the promise rejects', () => {
// Prepare
const records = [sqsRecordFactory('success'), sqsRecordFactory('success')];
const batch = { Records: records };
const processor = new BatchProcessorSync(EventType.SQS);

// Act & Assess
// The abandoned promise rejects after the throw, and vitest fails this file
// if nothing has taken ownership of that rejection.
expect(() =>
processPartialResponseSync(batch, rejectingRecordHandler, processor, {
context,
})
).toThrow(AsyncHandlerNotSupportedError);
});

it('throws when a SqsFifoPartialProcessor record handler returns a promise', () => {
// Prepare
const records = [sqsRecordFactory('success'), sqsRecordFactory('success')];
const batch = { Records: records };
const processor = new SqsFifoPartialProcessor();

// Act & Assess
expect(() =>
processPartialResponseSync(batch, asyncRecordHandler, processor, {
context,
})
).toThrow(AsyncHandlerNotSupportedError);
});
});