Skip to content

Commit 4420fad

Browse files
authored
fix: use failure event instead of error (#3219)
Ignore the error event as it causes `raceEvent` to throw. In future we should remove the error event from the queue as all we do with it is log the error, and if we want that, the `failure` event gives more context around the error so can make the logs more informative.
1 parent 79473c9 commit 4420fad

File tree

6 files changed

+22
-21
lines changed

6 files changed

+22
-21
lines changed

packages/kad-dht/src/content-routing/index.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,8 +134,8 @@ export class ContentRouting {
134134
queue.addEventListener('idle', () => {
135135
events.end()
136136
})
137-
queue.addEventListener('error', (err) => {
138-
this.log.error('error publishing provider record to peer - %e', err)
137+
queue.addEventListener('failure', (event) => {
138+
this.log.error('error publishing provider record to peer - %e', event.detail.error)
139139
})
140140

141141
queue.add(async () => {

packages/kad-dht/src/query/query-path.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,8 @@ export async function * queryPath (options: QueryPathOptions): AsyncGenerator<Qu
100100

101101
events.end()
102102
})
103-
queue.addEventListener('error', (evt) => {
104-
log.error('error during query - %e', evt.detail)
103+
queue.addEventListener('failure', (evt) => {
104+
log.error('error during query - %e', evt.detail.error)
105105
})
106106

107107
const onAbort = (): void => {

packages/libp2p/src/connection-manager/dial-queue.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,8 +101,8 @@ export class DialQueue {
101101
metrics: components.metrics
102102
})
103103
// a started job errored
104-
this.queue.addEventListener('error', (event) => {
105-
if (event.detail?.name !== AbortError.name) {
104+
this.queue.addEventListener('failure', (event) => {
105+
if (event.detail?.error.name !== AbortError.name) {
106106
this.log.error('error in dial queue - %e', event.detail)
107107
}
108108
})

packages/utils/src/queue/index.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,8 @@ export interface QueueEvents<JobReturnType, JobOptions extends AbortOptions = Ab
101101

102102
/**
103103
* A job has failed
104+
*
105+
* @deprecated Listen for the 'failure' event instead - it gives more context and is generally more useful, this event will be removed in a future release
104106
*/
105107
error: CustomEvent<Error>
106108

@@ -263,7 +265,6 @@ export class Queue<JobReturnType = unknown, JobOptions extends AbortOptions = Ab
263265
}
264266
}
265267

266-
this.safeDispatchEvent('error', { detail: err })
267268
this.safeDispatchEvent('failure', { detail: { job, error: err } })
268269

269270
throw err
@@ -395,8 +396,8 @@ export class Queue<JobReturnType = unknown, JobOptions extends AbortOptions = Ab
395396
}
396397
}
397398

398-
const onQueueError = (evt: CustomEvent<Error>): void => {
399-
cleanup(evt.detail)
399+
const onQueueFailure = (evt: CustomEvent<QueueJobFailure<JobReturnType, JobOptions>>): void => {
400+
cleanup(evt.detail.error)
400401
}
401402

402403
const onQueueIdle = (): void => {
@@ -410,7 +411,7 @@ export class Queue<JobReturnType = unknown, JobOptions extends AbortOptions = Ab
410411

411412
// add listeners
412413
this.addEventListener('completed', onQueueJobComplete)
413-
this.addEventListener('error', onQueueError)
414+
this.addEventListener('failure', onQueueFailure)
414415
this.addEventListener('idle', onQueueIdle)
415416
options?.signal?.addEventListener('abort', onSignalAbort)
416417

@@ -419,7 +420,7 @@ export class Queue<JobReturnType = unknown, JobOptions extends AbortOptions = Ab
419420
} finally {
420421
// remove listeners
421422
this.removeEventListener('completed', onQueueJobComplete)
422-
this.removeEventListener('error', onQueueError)
423+
this.removeEventListener('failure', onQueueFailure)
423424
this.removeEventListener('idle', onQueueIdle)
424425
options?.signal?.removeEventListener('abort', onSignalAbort)
425426

packages/utils/test/peer-job-queue.spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ describe('peer queue', () => {
165165
peerId: peerIdA
166166
}).catch(() => {})
167167

168-
const event = await raceEvent<CustomEvent<QueueJobFailure<string, PeerQueueJobOptions>>>(queue, 'failure')
168+
const event = await raceEvent<CustomEvent<QueueJobFailure<string, PeerQueueJobOptions>>>(queue, 'failure', AbortSignal.timeout(10_000))
169169

170170
expect(event.detail.job.options.peerId).to.equal(peerIdA)
171171
expect(event.detail.error).to.equal(err)

packages/utils/test/queue.spec.ts

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -508,10 +508,10 @@ describe('queue', () => {
508508
it('should emit completed / error events', async () => {
509509
const queue = new Queue({ concurrency: 1 })
510510

511-
let errorEvents = 0
511+
let failureEvents = 0
512512
let completedEvents = 0
513-
queue.addEventListener('error', () => {
514-
errorEvents++
513+
queue.addEventListener('failure', () => {
514+
failureEvents++
515515
})
516516
queue.addEventListener('completed', () => {
517517
completedEvents++
@@ -522,7 +522,7 @@ describe('queue', () => {
522522
expect(queue).to.have.property('size', 1)
523523
expect(queue).to.have.property('queued', 0)
524524
expect(queue).to.have.property('running', 1)
525-
expect(errorEvents).to.equal(0)
525+
expect(failureEvents).to.equal(0)
526526
expect(completedEvents).to.equal(0)
527527

528528
const job2 = queue.add(async () => {
@@ -533,39 +533,39 @@ describe('queue', () => {
533533
expect(queue).to.have.property('size', 2)
534534
expect(queue).to.have.property('queued', 1)
535535
expect(queue).to.have.property('running', 1)
536-
expect(errorEvents).to.equal(0)
536+
expect(failureEvents).to.equal(0)
537537
expect(completedEvents).to.equal(0)
538538

539539
await job1
540540

541541
expect(queue).to.have.property('size', 1)
542542
expect(queue).to.have.property('queued', 0)
543543
expect(queue).to.have.property('running', 1)
544-
expect(errorEvents).to.equal(0)
544+
expect(failureEvents).to.equal(0)
545545
expect(completedEvents).to.equal(1)
546546

547547
await expect(job2).to.eventually.be.rejected()
548548

549549
expect(queue).to.have.property('size', 0)
550550
expect(queue).to.have.property('queued', 0)
551551
expect(queue).to.have.property('running', 0)
552-
expect(errorEvents).to.equal(1)
552+
expect(failureEvents).to.equal(1)
553553
expect(completedEvents).to.equal(1)
554554

555555
const job3 = queue.add(async () => delay(100))
556556

557557
expect(queue).to.have.property('size', 1)
558558
expect(queue).to.have.property('queued', 0)
559559
expect(queue).to.have.property('running', 1)
560-
expect(errorEvents).to.equal(1)
560+
expect(failureEvents).to.equal(1)
561561
expect(completedEvents).to.equal(1)
562562

563563
await job3
564564

565565
expect(queue).to.have.property('size', 0)
566566
expect(queue).to.have.property('queued', 0)
567567
expect(queue).to.have.property('running', 0)
568-
expect(errorEvents).to.equal(1)
568+
expect(failureEvents).to.equal(1)
569569
expect(completedEvents).to.equal(2)
570570
})
571571

0 commit comments

Comments
 (0)