Skip to content

Conversation

@amanakin
Copy link
Contributor

@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

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 force-pushed the add_no_resolve_error_param branch from 20a5223 to a170ed1 Compare May 7, 2025 14:57
@amanakin amanakin force-pushed the add_no_resolve_error_param branch from d94f5e4 to 752326c Compare May 7, 2025 15:17
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

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

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
Contributor Author

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
Contributor Author

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

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants