fix: always catch and emit cache write errors in promise #288
Conversation
| @@ -274,6 +274,10 @@ class CacheEntry { | |||
| const cacheWritePromise = new Promise((resolve, reject) => { | |||
| cacheWriteResolve = resolve | |||
| cacheWriteReject = reject | |||
| }).catch((err) => { | |||
There was a problem hiding this comment.
This would be from when we called (reject) so why not emit it there?
This may be a nitpick.
There was a problem hiding this comment.
line 302
cacheStream.promise().then(cacheWriteResolve, cacheWriteReject)
the error function should emit imho.
cacheStream.promise().then(cacheWriteResolve, err => {
if (body) {
body.emit('error', err)
}
cacheWriteReject(err)
})
(typed on the website, syntax may be all wrong).
There was a problem hiding this comment.
Confirmed w/ nlf. The error is the lack of an emit when rejecting, we shouldn't need a catch we should just be sure to emit when rejecting. It's also more intuitive, it's harder to reason why we're doing a blanket catch versus having an emit before we reject.
I think this also eats the rejection.
There was a problem hiding this comment.
I agree with moving the emitting to line ~302.
if (body) {
We can drop the if. At line 302 we know there's a body. In fact we were called by the body's resume handler.
There was a problem hiding this comment.
So far I've learned there has to be a catch attached to the promise immediately, otherwise the cache dir creation error is unhandled. So if the catch needs to be there then we also need to emit from there. But I'm still not sold on my fix being the correct one, only that emitting from a different place does not fix the test case.
There was a problem hiding this comment.
Also dropped the if (body) statement as suggested since it was not necessary.
lib/cache/entry.js
Outdated
| @@ -274,6 +274,10 @@ class CacheEntry { | |||
| const cacheWritePromise = new Promise((resolve, reject) => { | |||
| cacheWriteResolve = resolve | |||
| cacheWriteReject = reject | |||
| }).catch((err) => { | |||
| if (body) { | |||
There was a problem hiding this comment.
What if there is no body? Do we still need to emit an error event so the stream is cleaned up?
There was a problem hiding this comment.
@hashtagchris pointed out that there will always be a body in this so I removed the if statement guard.
| }).catch((err) => { | ||
| if (body) { | ||
| body.emit('error', err) | ||
| } | ||
| }) | ||
|
|
||
| body = new CachingMinipassPipeline({ events: ['integrity', 'size'] }, new MinipassFlush({ |
There was a problem hiding this comment.
We don't need to declare the error event, do we?
| body = new CachingMinipassPipeline({ events: ['integrity', 'size'] }, new MinipassFlush({ | |
| body = new CachingMinipassPipeline({ events: ['integrity', 'size', 'error'] }, new MinipassFlush({ |
There was a problem hiding this comment.
We don't since that event is always raised on the stream and is not a new event that we need to inform the caching stream of like integrity and size.
| @@ -274,6 +274,10 @@ class CacheEntry { | |||
| const cacheWritePromise = new Promise((resolve, reject) => { | |||
| cacheWriteResolve = resolve | |||
| cacheWriteReject = reject | |||
| }).catch((err) => { | |||
There was a problem hiding this comment.
I agree with moving the emitting to line ~302.
if (body) {
We can drop the if. At line 302 we know there's a body. In fact we were called by the body's resume handler.
🤖 I have created a release *beep* *boop* --- ## [13.0.1](v13.0.0...v13.0.1) (2024-04-30) ### Bug Fixes * [`66018e3`](66018e3) log errors on retry (@wraithgar) * [`ed73ef5`](ed73ef5) [#288](#288) always catch and emit cache write errors in promise (#288) (@lukekarrys) ### Chores * [`9e1329c`](9e1329c) [#292](#292) fix linting in tests (@lukekarrys) * [`4756bda`](4756bda) [#292](#292) postinstall for dependabot template-oss PR (@lukekarrys) * [`91df666`](91df666) [#292](#292) bump @npmcli/template-oss from 4.21.3 to 4.21.4 (@dependabot[bot]) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
No description provided.