fix(astro): Handle errors in middlewares better

[mydea]

Hopefully fixes#16491

I could not get a reproducing test there for this 🤔 I figure this is "safe" to do, but not quite sure how/when this would happen. I would assume this was "introduced" by #15995, maybe @Fryuni has a clue how/when that could happen and if this is a reasonable change 🤔

[Fryuni]

The error description indicates that calling enqueue on a closed controller fails, not close. close should be a no-op on an already closed controller.

The controller may be closed by downstream code for multiple reasons, like a canceled request or a closed connection.
In theory once the request is closed we should stop the iterations and close the original stream. But we could just consume it to the end discarding everything.

I think (would have to look at the docs) there is a .closed property we could check before calling enqueue

[mydea]

ahh I see, that makes sense. Should we maybe simply not send errors from this to sentry, then 🤔 is there a value in doing this? So just:

try{forawait(constchunkoforiginalBody){consthtml=typeofchunk==='string' ? chunk : decoder.decode(chunk,{stream: true});constmodifiedHtml=addMetaTagToHead(html);controller.enqueue(newTextEncoder().encode(modifiedHtml));}}catch(e){controller.error(e);}finally{controller.close();}

would lose some visibility, but probably ok...? We could just logger.error the error instead, I suppose... this does not feel actionable for a user, I'd say 🤔 cc @AbhiPrasad

[Fryuni]

A bit tricky 🤔

The iterator can fail with a user error from a component, but the body of the loop can error out of the user's control.

We'd be switching from reporting unactionable errors to not reporting actionable ones.

If we hand roll the iteration using the interface and a while loop we could have the try/catch on each part with different error handlers.

[Fryuni]

Here is one way to avoid that:

// Assign to a new variable to avoid TS losing the narrower type checked above.constbody=originalBody;asyncfunction*bodyReporter(): AsyncGenerator<string|Buffer>{try{forawait(constchunkofbody){yieldchunk;}}catch(e){// Report stream errors coming from user code or Astro rendering.sendErrorToSentry(e);throwe;}}constreportedBody=bodyReporter();try{forawait(constchunkofreportedBody){consthtml=typeofchunk==='string' ? chunk : decoder.decode(chunk,{stream: true});constmodifiedHtml=addMetaTagToHead(html);controller.enqueue(newTextEncoder().encode(modifiedHtml));}}catch(e){controller.error(e);}finally{controller.close();}

[mydea]

nice, thank you, that makes sense to me! is there a good way to test that this actually works? 😅

[Fryuni]

Hum... A little tricky because this is deeply inside of Astro.

You can make a component with await timers.setTimeout(5000) in the frontmatter and then close the connection while waiting for it. It should cause the enqueue on a closed controller. On a unit test you just need to close the response body and produce a new chunk from the inner response after that.

[mydea]

Hmm I will just merge this as is for now, maybe we can add a test later!

[mydea]

Hopefully fixes#16491

I could not get a reproducing test there for this 🤔 I figure this is "safe" to do, but not quite sure how/when this would happen. I would assume this was "introduced" by #15995, maybe @Fryuni has a clue how/when that could happen and if this is a reasonable change 🤔

[Fryuni]

The error description indicates that calling enqueue on a closed controller fails, not close. close should be a no-op on an already closed controller.

The controller may be closed by downstream code for multiple reasons, like a canceled request or a closed connection.
In theory once the request is closed we should stop the iterations and close the original stream. But we could just consume it to the end discarding everything.

I think (would have to look at the docs) there is a .closed property we could check before calling enqueue

[mydea]

ahh I see, that makes sense. Should we maybe simply not send errors from this to sentry, then 🤔 is there a value in doing this? So just:

try{forawait(constchunkoforiginalBody){consthtml=typeofchunk==='string' ? chunk : decoder.decode(chunk,{stream: true});constmodifiedHtml=addMetaTagToHead(html);controller.enqueue(newTextEncoder().encode(modifiedHtml));}}catch(e){controller.error(e);}finally{controller.close();}

would lose some visibility, but probably ok...? We could just logger.error the error instead, I suppose... this does not feel actionable for a user, I'd say 🤔 cc @AbhiPrasad

[Fryuni]

A bit tricky 🤔

The iterator can fail with a user error from a component, but the body of the loop can error out of the user's control.

We'd be switching from reporting unactionable errors to not reporting actionable ones.

If we hand roll the iteration using the interface and a while loop we could have the try/catch on each part with different error handlers.

[Fryuni]

Here is one way to avoid that:

// Assign to a new variable to avoid TS losing the narrower type checked above.constbody=originalBody;asyncfunction*bodyReporter(): AsyncGenerator<string|Buffer>{try{forawait(constchunkofbody){yieldchunk;}}catch(e){// Report stream errors coming from user code or Astro rendering.sendErrorToSentry(e);throwe;}}constreportedBody=bodyReporter();try{forawait(constchunkofreportedBody){consthtml=typeofchunk==='string' ? chunk : decoder.decode(chunk,{stream: true});constmodifiedHtml=addMetaTagToHead(html);controller.enqueue(newTextEncoder().encode(modifiedHtml));}}catch(e){controller.error(e);}finally{controller.close();}

[mydea]

nice, thank you, that makes sense to me! is there a good way to test that this actually works? 😅

[Fryuni]

Hum... A little tricky because this is deeply inside of Astro.

You can make a component with await timers.setTimeout(5000) in the frontmatter and then close the connection while waiting for it. It should cause the enqueue on a closed controller. On a unit test you just need to close the response body and produce a new chunk from the inner response after that.

[mydea]

Hmm I will just merge this as is for now, maybe we can add a test later!

[mydea]

Hopefully fixes#16491

I could not get a reproducing test there for this 🤔 I figure this is "safe" to do, but not quite sure how/when this would happen. I would assume this was "introduced" by #15995, maybe @Fryuni has a clue how/when that could happen and if this is a reasonable change 🤔

[Fryuni]

The error description indicates that calling enqueue on a closed controller fails, not close. close should be a no-op on an already closed controller.

The controller may be closed by downstream code for multiple reasons, like a canceled request or a closed connection.
In theory once the request is closed we should stop the iterations and close the original stream. But we could just consume it to the end discarding everything.

I think (would have to look at the docs) there is a .closed property we could check before calling enqueue

[mydea]

ahh I see, that makes sense. Should we maybe simply not send errors from this to sentry, then 🤔 is there a value in doing this? So just:

try{forawait(constchunkoforiginalBody){consthtml=typeofchunk==='string' ? chunk : decoder.decode(chunk,{stream: true});constmodifiedHtml=addMetaTagToHead(html);controller.enqueue(newTextEncoder().encode(modifiedHtml));}}catch(e){controller.error(e);}finally{controller.close();}

would lose some visibility, but probably ok...? We could just logger.error the error instead, I suppose... this does not feel actionable for a user, I'd say 🤔 cc @AbhiPrasad

[Fryuni]

A bit tricky 🤔

The iterator can fail with a user error from a component, but the body of the loop can error out of the user's control.

We'd be switching from reporting unactionable errors to not reporting actionable ones.

If we hand roll the iteration using the interface and a while loop we could have the try/catch on each part with different error handlers.

[Fryuni]

Here is one way to avoid that:

// Assign to a new variable to avoid TS losing the narrower type checked above.constbody=originalBody;asyncfunction*bodyReporter(): AsyncGenerator<string|Buffer>{try{forawait(constchunkofbody){yieldchunk;}}catch(e){// Report stream errors coming from user code or Astro rendering.sendErrorToSentry(e);throwe;}}constreportedBody=bodyReporter();try{forawait(constchunkofreportedBody){consthtml=typeofchunk==='string' ? chunk : decoder.decode(chunk,{stream: true});constmodifiedHtml=addMetaTagToHead(html);controller.enqueue(newTextEncoder().encode(modifiedHtml));}}catch(e){controller.error(e);}finally{controller.close();}

[mydea]

nice, thank you, that makes sense to me! is there a good way to test that this actually works? 😅

[Fryuni]

Hum... A little tricky because this is deeply inside of Astro.

You can make a component with await timers.setTimeout(5000) in the frontmatter and then close the connection while waiting for it. It should cause the enqueue on a closed controller. On a unit test you just need to close the response body and produce a new chunk from the inner response after that.

[mydea]

Hmm I will just merge this as is for now, maybe we can add a test later!

[mydea]

Hopefully fixes#16491

I could not get a reproducing test there for this 🤔 I figure this is "safe" to do, but not quite sure how/when this would happen. I would assume this was "introduced" by #15995, maybe @Fryuni has a clue how/when that could happen and if this is a reasonable change 🤔

[Fryuni]

The error description indicates that calling enqueue on a closed controller fails, not close. close should be a no-op on an already closed controller.

The controller may be closed by downstream code for multiple reasons, like a canceled request or a closed connection.
In theory once the request is closed we should stop the iterations and close the original stream. But we could just consume it to the end discarding everything.

I think (would have to look at the docs) there is a .closed property we could check before calling enqueue

[mydea]

ahh I see, that makes sense. Should we maybe simply not send errors from this to sentry, then 🤔 is there a value in doing this? So just:

try{forawait(constchunkoforiginalBody){consthtml=typeofchunk==='string' ? chunk : decoder.decode(chunk,{stream: true});constmodifiedHtml=addMetaTagToHead(html);controller.enqueue(newTextEncoder().encode(modifiedHtml));}}catch(e){controller.error(e);}finally{controller.close();}

would lose some visibility, but probably ok...? We could just logger.error the error instead, I suppose... this does not feel actionable for a user, I'd say 🤔 cc @AbhiPrasad

[Fryuni]

A bit tricky 🤔

The iterator can fail with a user error from a component, but the body of the loop can error out of the user's control.

We'd be switching from reporting unactionable errors to not reporting actionable ones.

If we hand roll the iteration using the interface and a while loop we could have the try/catch on each part with different error handlers.

[Fryuni]

Here is one way to avoid that:

// Assign to a new variable to avoid TS losing the narrower type checked above.constbody=originalBody;asyncfunction*bodyReporter(): AsyncGenerator<string|Buffer>{try{forawait(constchunkofbody){yieldchunk;}}catch(e){// Report stream errors coming from user code or Astro rendering.sendErrorToSentry(e);throwe;}}constreportedBody=bodyReporter();try{forawait(constchunkofreportedBody){consthtml=typeofchunk==='string' ? chunk : decoder.decode(chunk,{stream: true});constmodifiedHtml=addMetaTagToHead(html);controller.enqueue(newTextEncoder().encode(modifiedHtml));}}catch(e){controller.error(e);}finally{controller.close();}

[mydea]

nice, thank you, that makes sense to me! is there a good way to test that this actually works? 😅

[Fryuni]

Hum... A little tricky because this is deeply inside of Astro.

You can make a component with await timers.setTimeout(5000) in the frontmatter and then close the connection while waiting for it. It should cause the enqueue on a closed controller. On a unit test you just need to close the response body and produce a new chunk from the inner response after that.

[mydea]

Hmm I will just merge this as is for now, maybe we can add a test later!

[mydea]

Hopefully fixes#16491

I could not get a reproducing test there for this 🤔 I figure this is "safe" to do, but not quite sure how/when this would happen. I would assume this was "introduced" by #15995, maybe @Fryuni has a clue how/when that could happen and if this is a reasonable change 🤔

[Fryuni]

The error description indicates that calling enqueue on a closed controller fails, not close. close should be a no-op on an already closed controller.

The controller may be closed by downstream code for multiple reasons, like a canceled request or a closed connection.
In theory once the request is closed we should stop the iterations and close the original stream. But we could just consume it to the end discarding everything.

I think (would have to look at the docs) there is a .closed property we could check before calling enqueue

[mydea]

ahh I see, that makes sense. Should we maybe simply not send errors from this to sentry, then 🤔 is there a value in doing this? So just:

try{forawait(constchunkoforiginalBody){consthtml=typeofchunk==='string' ? chunk : decoder.decode(chunk,{stream: true});constmodifiedHtml=addMetaTagToHead(html);controller.enqueue(newTextEncoder().encode(modifiedHtml));}}catch(e){controller.error(e);}finally{controller.close();}

would lose some visibility, but probably ok...? We could just logger.error the error instead, I suppose... this does not feel actionable for a user, I'd say 🤔 cc @AbhiPrasad

[Fryuni]

A bit tricky 🤔

The iterator can fail with a user error from a component, but the body of the loop can error out of the user's control.

We'd be switching from reporting unactionable errors to not reporting actionable ones.

If we hand roll the iteration using the interface and a while loop we could have the try/catch on each part with different error handlers.

[Fryuni]

Here is one way to avoid that:

// Assign to a new variable to avoid TS losing the narrower type checked above.constbody=originalBody;asyncfunction*bodyReporter(): AsyncGenerator<string|Buffer>{try{forawait(constchunkofbody){yieldchunk;}}catch(e){// Report stream errors coming from user code or Astro rendering.sendErrorToSentry(e);throwe;}}constreportedBody=bodyReporter();try{forawait(constchunkofreportedBody){consthtml=typeofchunk==='string' ? chunk : decoder.decode(chunk,{stream: true});constmodifiedHtml=addMetaTagToHead(html);controller.enqueue(newTextEncoder().encode(modifiedHtml));}}catch(e){controller.error(e);}finally{controller.close();}

[mydea]

nice, thank you, that makes sense to me! is there a good way to test that this actually works? 😅

[Fryuni]

Hum... A little tricky because this is deeply inside of Astro.

You can make a component with await timers.setTimeout(5000) in the frontmatter and then close the connection while waiting for it. It should cause the enqueue on a closed controller. On a unit test you just need to close the response body and produce a new chunk from the inner response after that.

[mydea]

Hmm I will just merge this as is for now, maybe we can add a test later!

[mydea]

Hopefully fixes#16491

I could not get a reproducing test there for this 🤔 I figure this is "safe" to do, but not quite sure how/when this would happen. I would assume this was "introduced" by #15995, maybe @Fryuni has a clue how/when that could happen and if this is a reasonable change 🤔

[Fryuni]

The error description indicates that calling enqueue on a closed controller fails, not close. close should be a no-op on an already closed controller.

The controller may be closed by downstream code for multiple reasons, like a canceled request or a closed connection.
In theory once the request is closed we should stop the iterations and close the original stream. But we could just consume it to the end discarding everything.

I think (would have to look at the docs) there is a .closed property we could check before calling enqueue

[mydea]

ahh I see, that makes sense. Should we maybe simply not send errors from this to sentry, then 🤔 is there a value in doing this? So just:

try{forawait(constchunkoforiginalBody){consthtml=typeofchunk==='string' ? chunk : decoder.decode(chunk,{stream: true});constmodifiedHtml=addMetaTagToHead(html);controller.enqueue(newTextEncoder().encode(modifiedHtml));}}catch(e){controller.error(e);}finally{controller.close();}

would lose some visibility, but probably ok...? We could just logger.error the error instead, I suppose... this does not feel actionable for a user, I'd say 🤔 cc @AbhiPrasad

[Fryuni]

A bit tricky 🤔

The iterator can fail with a user error from a component, but the body of the loop can error out of the user's control.

We'd be switching from reporting unactionable errors to not reporting actionable ones.

If we hand roll the iteration using the interface and a while loop we could have the try/catch on each part with different error handlers.

[Fryuni]

Here is one way to avoid that:

// Assign to a new variable to avoid TS losing the narrower type checked above.constbody=originalBody;asyncfunction*bodyReporter(): AsyncGenerator<string|Buffer>{try{forawait(constchunkofbody){yieldchunk;}}catch(e){// Report stream errors coming from user code or Astro rendering.sendErrorToSentry(e);throwe;}}constreportedBody=bodyReporter();try{forawait(constchunkofreportedBody){consthtml=typeofchunk==='string' ? chunk : decoder.decode(chunk,{stream: true});constmodifiedHtml=addMetaTagToHead(html);controller.enqueue(newTextEncoder().encode(modifiedHtml));}}catch(e){controller.error(e);}finally{controller.close();}

[mydea]

nice, thank you, that makes sense to me! is there a good way to test that this actually works? 😅

[Fryuni]

Hum... A little tricky because this is deeply inside of Astro.

You can make a component with await timers.setTimeout(5000) in the frontmatter and then close the connection while waiting for it. It should cause the enqueue on a closed controller. On a unit test you just need to close the response body and produce a new chunk from the inner response after that.

[mydea]

Hmm I will just merge this as is for now, maybe we can add a test later!

[mydea]

Hopefully fixes#16491

I could not get a reproducing test there for this 🤔 I figure this is "safe" to do, but not quite sure how/when this would happen. I would assume this was "introduced" by #15995, maybe @Fryuni has a clue how/when that could happen and if this is a reasonable change 🤔

[Fryuni]

The error description indicates that calling enqueue on a closed controller fails, not close. close should be a no-op on an already closed controller.

The controller may be closed by downstream code for multiple reasons, like a canceled request or a closed connection.
In theory once the request is closed we should stop the iterations and close the original stream. But we could just consume it to the end discarding everything.

I think (would have to look at the docs) there is a .closed property we could check before calling enqueue

[mydea]

ahh I see, that makes sense. Should we maybe simply not send errors from this to sentry, then 🤔 is there a value in doing this? So just:

try{forawait(constchunkoforiginalBody){consthtml=typeofchunk==='string' ? chunk : decoder.decode(chunk,{stream: true});constmodifiedHtml=addMetaTagToHead(html);controller.enqueue(newTextEncoder().encode(modifiedHtml));}}catch(e){controller.error(e);}finally{controller.close();}

would lose some visibility, but probably ok...? We could just logger.error the error instead, I suppose... this does not feel actionable for a user, I'd say 🤔 cc @AbhiPrasad

[Fryuni]

A bit tricky 🤔

The iterator can fail with a user error from a component, but the body of the loop can error out of the user's control.

We'd be switching from reporting unactionable errors to not reporting actionable ones.

If we hand roll the iteration using the interface and a while loop we could have the try/catch on each part with different error handlers.

[Fryuni]

Here is one way to avoid that:

// Assign to a new variable to avoid TS losing the narrower type checked above.constbody=originalBody;asyncfunction*bodyReporter(): AsyncGenerator<string|Buffer>{try{forawait(constchunkofbody){yieldchunk;}}catch(e){// Report stream errors coming from user code or Astro rendering.sendErrorToSentry(e);throwe;}}constreportedBody=bodyReporter();try{forawait(constchunkofreportedBody){consthtml=typeofchunk==='string' ? chunk : decoder.decode(chunk,{stream: true});constmodifiedHtml=addMetaTagToHead(html);controller.enqueue(newTextEncoder().encode(modifiedHtml));}}catch(e){controller.error(e);}finally{controller.close();}

[mydea]

nice, thank you, that makes sense to me! is there a good way to test that this actually works? 😅

[Fryuni]

Hum... A little tricky because this is deeply inside of Astro.

You can make a component with await timers.setTimeout(5000) in the frontmatter and then close the connection while waiting for it. It should cause the enqueue on a closed controller. On a unit test you just need to close the response body and produce a new chunk from the inner response after that.

[mydea]

Hmm I will just merge this as is for now, maybe we can add a test later!

[mydea]

Hopefully fixes#16491

I could not get a reproducing test there for this 🤔 I figure this is "safe" to do, but not quite sure how/when this would happen. I would assume this was "introduced" by #15995, maybe @Fryuni has a clue how/when that could happen and if this is a reasonable change 🤔

[Fryuni]

The error description indicates that calling enqueue on a closed controller fails, not close. close should be a no-op on an already closed controller.

The controller may be closed by downstream code for multiple reasons, like a canceled request or a closed connection.
In theory once the request is closed we should stop the iterations and close the original stream. But we could just consume it to the end discarding everything.

I think (would have to look at the docs) there is a .closed property we could check before calling enqueue

[mydea]

ahh I see, that makes sense. Should we maybe simply not send errors from this to sentry, then 🤔 is there a value in doing this? So just:

try{forawait(constchunkoforiginalBody){consthtml=typeofchunk==='string' ? chunk : decoder.decode(chunk,{stream: true});constmodifiedHtml=addMetaTagToHead(html);controller.enqueue(newTextEncoder().encode(modifiedHtml));}}catch(e){controller.error(e);}finally{controller.close();}

would lose some visibility, but probably ok...? We could just logger.error the error instead, I suppose... this does not feel actionable for a user, I'd say 🤔 cc @AbhiPrasad

[Fryuni]

A bit tricky 🤔

The iterator can fail with a user error from a component, but the body of the loop can error out of the user's control.

We'd be switching from reporting unactionable errors to not reporting actionable ones.

If we hand roll the iteration using the interface and a while loop we could have the try/catch on each part with different error handlers.

[Fryuni]

Here is one way to avoid that:

// Assign to a new variable to avoid TS losing the narrower type checked above.constbody=originalBody;asyncfunction*bodyReporter(): AsyncGenerator<string|Buffer>{try{forawait(constchunkofbody){yieldchunk;}}catch(e){// Report stream errors coming from user code or Astro rendering.sendErrorToSentry(e);throwe;}}constreportedBody=bodyReporter();try{forawait(constchunkofreportedBody){consthtml=typeofchunk==='string' ? chunk : decoder.decode(chunk,{stream: true});constmodifiedHtml=addMetaTagToHead(html);controller.enqueue(newTextEncoder().encode(modifiedHtml));}}catch(e){controller.error(e);}finally{controller.close();}

[mydea]

nice, thank you, that makes sense to me! is there a good way to test that this actually works? 😅

[Fryuni]

Hum... A little tricky because this is deeply inside of Astro.

You can make a component with await timers.setTimeout(5000) in the frontmatter and then close the connection while waiting for it. It should cause the enqueue on a closed controller. On a unit test you just need to close the response body and produce a new chunk from the inner response after that.

[mydea]

Hmm I will just merge this as is for now, maybe we can add a test later!