Skip to content

Fix memory leak of AudioSource#486

Merged
typester merged 3 commits intomainfrom
track-close
Jun 16, 2025
Merged

Fix memory leak of AudioSource#486
typester merged 3 commits intomainfrom
track-close

Conversation

@typester
Copy link
Copy Markdown
Contributor

@typester typester commented Jun 2, 2025

It's actually an issue on Track side where it keep a reference for AudioSource and needs to be disposed

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Jun 2, 2025

🦋 Changeset detected

Latest commit: 523f0f4

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@typester typester marked this pull request as ready for review June 9, 2025 19:12
@typester typester requested review from bcherry and lukasIO June 9, 2025 19:12
@typester
Copy link
Copy Markdown
Contributor Author

typester commented Jun 9, 2025

I'm going to apply this fix to agent-js after this fix is released. Also I will do the documentation improvement on that.

@davidzhao davidzhao requested a review from Shubhrakanti June 9, 2025 19:44
Copy link
Copy Markdown
Contributor

@Shubhrakanti Shubhrakanti left a comment

Choose a reason for hiding this comment

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

This makes sense to me.

return this.info?.muted;
}

async close() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we have a optional boolean here to also close the source when closing the track? e.g.
async close(closeSource = true) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Shubhrakanti @bcherry I'm not familiar with JS side but this sounds fair to me. What do you think?

I'm going to merge this pr but I'm open to implement this in a different pr

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This makes sense to me!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I will create an other pr for it!

@typester
Copy link
Copy Markdown
Contributor Author

The CI for Windows failed. Re-running is no luck. But I tried it with main branch, it failed either. So I don't think this pr breaks it. I will merge this but will need to investigate. It seems that it's the same issue with we have rust-sdks. I thought this was GitHub issue, but I'm now thinking we might have some race conditions in our build system

@typester typester merged commit e1a0780 into main Jun 16, 2025
52 of 56 checks passed
@typester typester deleted the track-close branch June 16, 2025 17:06
@github-actions github-actions bot mentioned this pull request Jun 23, 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