Add client config param IgnoreTURNResolveErrors #455

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Draft

amanakin wants to merge 2 commits into pion:master

from amanakin:add_no_resolve_error_param

Draft

Add client config param IgnoreTURNResolveErrors #455

amanakin wants to merge 2 commits into pion:master from amanakin:add_no_resolve_error_param

Conversation

@amanakin

Copy link

Member

@amanakin amanakin commented May 7, 2025

Description

In some configurations we don't use the TURN address at the client side (e.g. proxy connection, see #450). But now at the client setup the provided turn address must be resolvable.

Adding client config param IgnoreTURNResolveErrors that allows omit resolving errors when we don't need turn address.

Also discussed here: pion/ice#773

@codecov

Copy link

codecov bot commented May 7, 2025 •

edited

Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.57%. Comparing base (51f0d62) to head (752326c).

Additional details and impacted files

@@ Coverage Diff @@
## master #455 +/- ##
==========================================
+ Coverage 67.43% 67.57% +0.13% 
==========================================
 Files 43 43 
 Lines 3083 3087 +4 
==========================================
+ Hits 2079 2086 +7 
+ Misses 843 841 -2 
+ Partials 161 160 -1

Flag	Coverage Δ
go	`67.57% <100.00%> (+0.13%)`	⬆️
wasm	`26.36% <0.00%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@amanakin amanakin mentioned this pull request

May 7, 2025

Avoid unnecessary turn address resolution in the turn client pion/ice#773

Open

@amanakin


 Add client config param IgnoreTURNResolveErrors

a170ed1

@amanakin amanakin force-pushed the add_no_resolve_error_param branch from 20a5223 to a170ed1 Compare

May 7, 2025 14:57

@amanakin


 Add client turn addresss resolving test

752326c

@amanakin amanakin force-pushed the add_no_resolve_error_param branch from d94f5e4 to 752326c Compare

May 7, 2025 15:17

JoTurk

JoTurk reviewed

May 7, 2025

View reviewed changes

client.go

Net transport.Net

LoggerFactory logging.LoggerFactory

IgnoreTURNResolveErrors bool // TURN server address is not required for some configurations (e.g. proxy)

Copy link

Member

Choose a reason for hiding this comment

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

I don't think we should add a config, there are many reason, but it feels like a single use flag.
What if we make a new constructor NewClientWithResolver and essentially make the nested condition after if len(config.TURNServerAddr) > 0 { customizable, we can then make the current NewClient call this new constructor with the old resolver logic.

Copy link

Member

@JoTurk JoTurk May 7, 2025

Choose a reason for hiding this comment

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

Even better, we can use Pion options, pattern, and make NewClientWithOptions and make a custom option WithResolver.

Copy link

Member

@JoTurk JoTurk May 7, 2025

Choose a reason for hiding this comment

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

I can make a PR for this!

Copy link

Member Author

@amanakin amanakin May 7, 2025

Choose a reason for hiding this comment

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

@joeturki So you mean something like

if len(config.TURNServerAddr) > 0 {
	turnServ, err = config.ResolveTURN(config.TURNServerAddr)
	if err != nil {
		return nil, err
	}
}

And then in some cases use there WithResolveTURN(...) that will return nil, nil?

Copy link

Member Author

@amanakin amanakin May 22, 2025

Choose a reason for hiding this comment

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

@joeturki Any updates on this?

Copy link

Member

@JoTurk JoTurk Oct 6, 2025

Choose a reason for hiding this comment

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

@amanakin hello, sorry, I didn't get a notification for this, do you still wanna merge this?

Labels

None yet

2 participants

@amanakin @JoTurk

Add client config param IgnoreTURNResolveErrors #455

Are you sure you want to change the base?

Add client config param IgnoreTURNResolveErrors #455

Uh oh!

Conversation

@amanakin amanakin commented May 7, 2025

Description

Uh oh!

codecov bot commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

@JoTurk JoTurk May 7, 2025

Choose a reason for hiding this comment

Uh oh!

@JoTurk JoTurk May 7, 2025

Choose a reason for hiding this comment

Uh oh!

@JoTurk JoTurk May 7, 2025

Choose a reason for hiding this comment

Uh oh!

@amanakin amanakin May 7, 2025

Choose a reason for hiding this comment

Uh oh!

@amanakin amanakin May 22, 2025

Choose a reason for hiding this comment

Uh oh!

@JoTurk JoTurk Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

codecov bot commented May 7, 2025 •

edited

Loading