Skip to content

Conversation

@andreas-karlsson
Copy link
Contributor

No description provided.

Copy link
Member

@fabriziodemaria fabriziodemaria left a comment

Choose a reason for hiding this comment

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

It's a bit outside my realm of expertise, but it looks good to me. I was mostly focused on cases of irrecoverable panics, ensuring there is minimal impact on the hosting application in that case

Comment on lines +127 to +130
private reloadInstance(error:unknown) {
logger.error('Failure calling into wasm:', error);
try {
this.bufferedLogs.push(this.delegate.flushLogs());
Copy link
Member

Choose a reason for hiding this comment

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

What happens with unrecoverable errors? I am thinking if the wasm doesn't restart and we keep pushing data to the bufferedLogs we might have a memory leak, perhaps cap the bufferedLogs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only thing that can produce logs is a successful resolve. And what's happening here is that we try once to extract logs from a broken instance before we throw it away. I don't think it's a problem from a memory leak perspective, i.e. this can't cause logs to grow faster than what successful operation does.

@nicklasl nicklasl changed the title feat: handle panics fix: handle panics Oct 24, 2025
@nicklasl nicklasl merged commit 1ea86ea into main Oct 24, 2025
4 checks passed
@nicklasl nicklasl deleted the handle-panics branch October 24, 2025 09:51
@github-actions github-actions bot mentioned this pull request Oct 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants