-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add StartContext to ICE and DTLS Transport #3371
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
base: main
Are you sure you want to change the base?
Changes from all commits
c9d2f4c
735079c
020958a
402eac2
89ec395
96db890
deb4189
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,7 +87,20 @@ func NewICETransport(gatherer *ICEGatherer, loggerFactory logging.LoggerFactory) | |
| } | ||
|
|
||
| // Start incoming connectivity checks based on its configured role. | ||
| func (t *ICETransport) Start(gatherer *ICEGatherer, params ICEParameters, role *ICERole) error { //nolint:cyclop | ||
| func (t *ICETransport) Start(gatherer *ICEGatherer, params ICEParameters, role *ICERole) error { | ||
| return t.StartContext(context.Background(), gatherer, params, role) | ||
| } | ||
|
|
||
| // StartContext incoming connectivity checks based on its configured role. | ||
| // If the context is canceled, the ICE transport will stop. | ||
| // | ||
| //nolint:cyclop | ||
| func (t *ICETransport) StartContext( | ||
| ctx context.Context, | ||
| gatherer *ICEGatherer, | ||
| params ICEParameters, | ||
| role *ICERole, | ||
| ) error { | ||
| t.lock.Lock() | ||
| defer t.lock.Unlock() | ||
|
|
||
|
|
@@ -134,7 +147,7 @@ func (t *ICETransport) Start(gatherer *ICEGatherer, params ICEParameters, role * | |
| } | ||
| t.role = *role | ||
|
|
||
| ctx, ctxCancel := context.WithCancel(context.Background()) | ||
| ctx, ctxCancel := context.WithCancel(ctx) | ||
| t.ctxCancel = ctxCancel | ||
|
|
||
|
Comment on lines
98
to
152
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a problem in the current ICE design, but it affects this PR, currently https://github.com/pion/ice/blob/main/transport.go ICE just returns an error if the context is cancelled after the connection checks started, and it doesn't call agent's close by itself (we should change in the next major version). But for this PR we should also call close when the context get cancelled.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be good now |
||
| // Drop the lock here to allow ICE candidates to be | ||
|
|
@@ -161,6 +174,17 @@ func (t *ICETransport) Start(gatherer *ICEGatherer, params ICEParameters, role * | |
| // Reacquire the lock to set the connection/mux | ||
| t.lock.Lock() | ||
| if err != nil { | ||
| if ctxErr := ctx.Err(); ctxErr != nil { | ||
| t.lock.Unlock() | ||
| _ = t.Stop() | ||
| t.lock.Lock() | ||
|
|
||
| return ctxErr | ||
| } | ||
|
Comment on lines
+177
to
+183
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sirzooro I don't think we would want a Pion api to return aribitary erros espically if this is used with third party created contexts, this will be kinda unpredicatable. and also sounds like a policy or API change across all of pion in a new major version and not just a single API and keep the rest.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be useful when used with complex context trees. Cancellation cause set few levels up the tree is propagated down when context is cancelled. This allows to improve logs, it is immediately obvious that connection was terminated due to something what happened elsewhere. Generic error "context cancelled" does not carry this information.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see the point, but it's outside of the scope of this change, we shouldn't introduce a new pattern or error handling randomly in a single new API and keep the rest. Because having unpredictable APIs and maintaining different behaviors is way worse than losing some context, and If we would do it, we should do it across all of pion and we should use a linter to force it, and it should be in a new release.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I am ok with this. |
||
|
|
||
| ctxCancel() | ||
| t.ctxCancel = nil | ||
|
|
||
| return err | ||
| } | ||
|
HashimTheArab marked this conversation as resolved.
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry can we revert this and check if t.api is null? calls with
t.apinull shouldn't panic.