Skip to content
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

Remove direct usage of unexported fields in ICETransport #659

Merged
merged 1 commit into from
May 9, 2019

Conversation

masterada
Copy link
Contributor

There are some direct usages of unexported fields of ICETransport
from non ice-related methods. This would be problematic when ice
once ice related code is moved to a separate packet. Added proxy
methods to ICETransport to avoid this.

Reference issue

Rel #646

@masterada masterada force-pushed the icetransport-unexported-field-usage branch 3 times, most recently from 42bb6e5 to 8e1927c Compare May 6, 2019 16:41
@Sean-Der
Copy link
Member

Sean-Der commented May 6, 2019

Hey @masterada

Want to tag+bump pion/ice and rebase off of that? This has that annoying PreICE fail :/

@masterada masterada force-pushed the icetransport-unexported-field-usage branch from 8e1927c to afaeb17 Compare May 6, 2019 18:50
@masterada
Copy link
Contributor Author

you got to be kidding me :D

@Sean-Der Sean-Der force-pushed the icetransport-unexported-field-usage branch from afaeb17 to 66105ae Compare May 6, 2019 21:36
icetransport.go Outdated
}

return nil
if t.state != ICETransportStateClosed {

This comment was marked as resolved.

This comment was marked as resolved.

Copy link
Member

Choose a reason for hiding this comment

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

@masterada correct me if I am wrong, but this feels a bit tech debty to me. I think this would possibly be the right solution (and I can help!)

  • Move ICETransportState to a atomic. Then we don't have to worry about mutex juggling
  • Fix pion/ice to properly emit state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

atomic - good idea
fix pion/ice - already created an issue for that and assigned myself, although im not sure when i will have time for it, not sure how hard it will be to write tests for it

there is still a question: do we need to make sure that once Close() call returns, State() will return closed (because if we wait for the agent's event to set the closed state for the transport, this will not be the case)

Copy link
Member

@Sean-Der Sean-Der left a comment

Choose a reason for hiding this comment

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

One minor change! Otherwise LGTM!

@masterada masterada force-pushed the icetransport-unexported-field-usage branch from 66105ae to feb2030 Compare May 7, 2019 07:56
@Sean-Der Sean-Der force-pushed the icetransport-unexported-field-usage branch 4 times, most recently from b6c1315 to 9d611ba Compare May 9, 2019 16:00
There are some direct usages of unexported fields of ICETransport
from non ice-related methods. This would be problematic when ice
once ice related code is moved to a separate packet. Added proxy
methods to ICETransport to avoid this.
@Sean-Der Sean-Der force-pushed the icetransport-unexported-field-usage branch from 9d611ba to 9f2bd3a Compare May 9, 2019 16:21
@masterada masterada merged commit 736e0bb into master May 9, 2019
@Sean-Der Sean-Der deleted the icetransport-unexported-field-usage branch May 12, 2019 07:21
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