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
19 changes: 14 additions & 5 deletions packages/amqp/lib/AbstractAmqpPublisher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,21 @@ export abstract class AbstractAmqpPublisher<
* it is intentional that this promise is not awaited, that's how we keep the method invocation synchronous
* RabbitMQ publish by itself doesn't guarantee that your message is delivered successfully, so this kind of fire-and-forget is not strongly different from how amqp-lib behaves in the first place.
*/
this.initPromise
.then(() => {
this.publish(message, options)
})
const initPromise = this.initPromise
initPromise
.then(
() => {
this.publish(message, options)
},
(err) => {
// Init failures only, not errors from the publish above: cleared so the next publish
// retries the init instead of re-awaiting the same rejection. Only if still ours: an
// earlier failed publish may already have started a retry.
if (this.initPromise === initPromise) this.initPromise = undefined
throw err
},
)
.catch((err) => {
Comment thread
ihlokalise marked this conversation as resolved.
/* v8 ignore next */
this.handleError(err)
})
return
Expand Down
46 changes: 45 additions & 1 deletion packages/amqp/test/publishers/AmqpPermissionPublisher.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import type { Channel } from 'amqplib'
import type { AwilixContainer } from 'awilix'
import { asClass, asFunction, Lifetime } from 'awilix'
import { asMockFunction } from 'awilix-manager'
import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from 'vitest'
import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'
import { ZodError } from 'zod/v4'
import { AbstractAmqpQueuePublisher } from '../../lib/AbstractAmqpQueuePublisher.ts'
import { deserializeAmqpMessage } from '../../lib/amqpMessageDeserializer.ts'
Expand Down Expand Up @@ -342,6 +342,50 @@ describe('PermissionPublisher', () => {
})
})

it('retries lazy init on the next publish after a failed one', async () => {
await permissionConsumer.close()
const newPublisher = new AmqpPermissionPublisher(diContainer.cradle)
const initSpy = vi
.spyOn(newPublisher, 'init')
.mockRejectedValueOnce(new Error('Transient init failure'))
const handleErrorSpy = vi.spyOn(newPublisher as any, 'handleError')

const message = {
id: '1',
messageType: 'add',
userIds: [1],
permissions: ['100'],
timestamp: new Date(),
} satisfies PERMISSIONS_MESSAGE_TYPE

newPublisher.publish(message)
await vi.waitFor(() => {
expect(handleErrorSpy).toHaveBeenCalledWith(new Error('Transient init failure'))
})

newPublisher.publish(message)
await vi.waitFor(() => {
expect(initSpy).toHaveBeenCalledTimes(2)
})

let receivedMessage: unknown
await channel.consume(AmqpPermissionPublisher.QUEUE_NAME, (message) => {
if (message === null) {
return
}
receivedMessage = deserializeAmqpMessage(
message,
PERMISSIONS_MESSAGE_SCHEMA,
new FakeConsumerErrorResolver(),
).result
})

await waitAndRetry(() => !!receivedMessage)
expect(receivedMessage).toMatchObject({ parsedMessage: { id: '1', messageType: 'add' } })

await newPublisher.close()
})

it('publishes a message auto-filling internal properties', async () => {
await permissionConsumer.close()

Expand Down
10 changes: 8 additions & 2 deletions packages/gcp-pubsub/lib/pubsub/AbstractPubSubPublisher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,14 @@ export abstract class AbstractPubSubPublisher<MessagePayloadType extends object>
if (!this.initPromise) {
this.initPromise = this.init()
}
await this.initPromise
this.initPromise = undefined
const initPromise = this.initPromise
try {
await initPromise
} finally {
// Cleared on failure too, so the next publish retries the init instead of re-awaiting the
// same rejection. Only if still ours: a concurrent caller may already have started a retry.
if (this.initPromise === initPromise) this.initPromise = undefined
}
}

try {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { PubSub } from '@google-cloud/pubsub'
import { afterAll, beforeAll, describe, expect, it } from 'vitest'
import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'

import { deletePubSubTopic } from '../utils/cleanupPubSub.ts'
import { registerDependencies } from '../utils/testContext.ts'
Expand Down Expand Up @@ -58,6 +58,29 @@ describe('PubSubPermissionPublisher', () => {
})

describe('publish', () => {
it('retries lazy init on the next publish after a failed one', async () => {
const newPublisher = new PubSubPermissionPublisher(diContainer.cradle)
const initSpy = vi
.spyOn(newPublisher, 'init')
.mockRejectedValueOnce(new Error('Transient init failure'))

const message = {
id: 'lazy-init-retry',
messageType: 'add' as const,
timestamp: new Date().toISOString(),
userIds: ['userId1'],
}

await expect(newPublisher.publish(message)).rejects.toThrow('Transient init failure')
await newPublisher.publish(message)

const spy = await newPublisher.handlerSpy.waitForMessageWithId('lazy-init-retry', 'published')
expect(spy.processingResult).toEqual({ status: 'published' })
expect(initSpy).toHaveBeenCalledTimes(2)

await newPublisher.close()
})

it('publishes a message to topic', async () => {
const message = {
id: '1',
Expand Down
10 changes: 8 additions & 2 deletions packages/sns/lib/sns/AbstractSnsPublisher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,14 @@ export abstract class AbstractSnsPublisher<MessagePayloadType extends object>
if (!this.initPromise) {
this.initPromise = this.init()
}
await this.initPromise
this.initPromise = undefined
const initPromise = this.initPromise
try {
await initPromise
} finally {
// Cleared on failure too, so the next publish retries the init instead of re-awaiting the
// same rejection. Only if still ours: a concurrent caller may already have started a retry.
if (this.initPromise === initPromise) this.initPromise = undefined
}
}

try {
Expand Down
21 changes: 21 additions & 0 deletions packages/sns/test/publishers/SnsPermissionPublisher.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -387,5 +387,26 @@ describe('SnsPermissionPublisher', () => {
const res = await newPublisher.handlerSpy.waitForMessageWithId('1', 'published')
expect(res.message).toEqual(message)
})

it('retries lazy init on the next publish after a failed one', async () => {
Comment thread
ihlokalise marked this conversation as resolved.
const newPublisher = new SnsPermissionPublisher(diContainer.cradle)
const initSpy = vi
.spyOn(newPublisher, 'init')
.mockRejectedValueOnce(new Error('Transient init failure'))

const message = {
id: '1',
messageType: 'add',
} satisfies PERMISSIONS_ADD_MESSAGE_TYPE

await expect(newPublisher.publish(message)).rejects.toThrow('Transient init failure')
await newPublisher.publish(message)

const res = await newPublisher.handlerSpy.waitForMessageWithId('1', 'published')
expect(res.message).toEqual(message)
expect(initSpy).toHaveBeenCalledTimes(2)

await newPublisher.close()
})
})
})
10 changes: 8 additions & 2 deletions packages/sqs/lib/sqs/AbstractSqsPublisher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,14 @@ export abstract class AbstractSqsPublisher<MessagePayloadType extends object>
if (!this.initPromise) {
this.initPromise = this.init()
}
await this.initPromise
this.initPromise = undefined
const initPromise = this.initPromise

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not caused by this PR, but it's close to it: in SQS, SNS and PubSub, close() doesn't clear initPromise (AMQP does). If close() runs while a lazy init is still going, the init finishes afterwards and sets isInitted = true again, so the closed publisher looks ready. A later publish() could also wait on the old promise.

Fine to leave it for a follow-up if you prefer. I just wanted to mention it since we're touching this code.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@CarlosGamero How we would want publish() to behave when close() is triggered? Should we reject publishing or still send?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good question! I’d keep today’s behaviour: after close(), isInitted is false, so the next publish() just runs the lazy init again and sends. Rejecting would be a breaking change and isn’t related to this fix.

With that in mind, the case I mentioned ends up almost the same as a normal re-init, so it’s low risk. The only small thing would be clearing initPromise in close() like AMQP already does, to keep the publishers consistent, but that’s optional and fine for a follow-up. Not a blocker for this PR 👍

try {
await initPromise
} finally {
// Cleared on failure too, so the next publish retries the init instead of re-awaiting the
// same rejection. Only if still ours: a concurrent caller may already have started a retry.
if (this.initPromise === initPromise) this.initPromise = undefined
}
}

try {
Expand Down
65 changes: 65 additions & 0 deletions packages/sqs/test/publishers/SqsPermissionPublisher.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -502,5 +502,70 @@ describe('SqsPermissionPublisher', () => {
expect(spy.message).toEqual(message)
expect(spy.processingResult).toEqual({ status: 'published' })
})

it('retries lazy init on the next publish after a failed one', async () => {
Comment thread
ihlokalise marked this conversation as resolved.
const newPublisher = new SqsPermissionPublisher(diContainer.cradle)
const initSpy = vi
.spyOn(newPublisher, 'init')
.mockRejectedValueOnce(new Error('Transient init failure'))

const message = {
id: '1',
messageType: 'add',
} satisfies PERMISSIONS_ADD_MESSAGE_TYPE

await expect(newPublisher.publish(message)).rejects.toThrow('Transient init failure')
await newPublisher.publish(message)

const spy = await newPublisher.handlerSpy.waitForMessageWithId('1', 'published')
expect(spy.processingResult).toEqual({ status: 'published' })
expect(initSpy).toHaveBeenCalledTimes(2)

await newPublisher.close()
})

it('retries lazy init once the missing queue exists', async () => {
const queueName = 'lazy_init_retry_queue'
await diContainer.cradle.testAdmin.deleteQueues(queueName)
const newPublisher = new SqsPermissionPublisher(diContainer.cradle, {
locatorConfig: { queueName },
})

const message = {
id: '1',
messageType: 'add',
} satisfies PERMISSIONS_ADD_MESSAGE_TYPE

await expect(newPublisher.publish(message)).rejects.toThrow(/does not exist/)

await diContainer.cradle.testAdmin.createQueue(queueName)
await newPublisher.publish(message)

const spy = await newPublisher.handlerSpy.waitForMessageWithId('1', 'published')
expect(spy.processingResult).toEqual({ status: 'published' })

await newPublisher.close()
await diContainer.cradle.testAdmin.deleteQueues(queueName)
})

it('shares one failed lazy init between concurrent publishes', async () => {
const newPublisher = new SqsPermissionPublisher(diContainer.cradle)
const initSpy = vi
.spyOn(newPublisher, 'init')
.mockRejectedValueOnce(new Error('Transient init failure'))

const results = await Promise.allSettled([
newPublisher.publish({ id: '1', messageType: 'add' }),
newPublisher.publish({ id: '2', messageType: 'add' }),
])

expect(results).toEqual([
{ status: 'rejected', reason: new Error('Transient init failure') },
{ status: 'rejected', reason: new Error('Transient init failure') },
])
expect(initSpy).toHaveBeenCalledTimes(1)

await newPublisher.close()
})
})
})
Loading