Skip to content
Snippets Groups Projects

Moved creation of PeerConnection to offerRequestSuccess

Merged HashikD requested to merge HashikD/snowflake-mobile:peerconnecion-refactor into master

closes #15 (closed)

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • We're also going to need to change where we close this peer connection. Right now this is done in the persistent service's OnDestroy method:

                mainPeerConnection.close();
                mainPeerConnection.dispose();
  • Hmm actually it does look like we're also closing it in closeConnectionAndResend. Do we need to dispose it here as well?

  • Author Maintainer

    hey @cohosh I am not sure what you mean? the OnDestroy is called by OS when it's killing off the service so we have to use it there. However, in !4 (closed) I refactored the closeConnectionAndResend to closeConnection and added a flag as an input to check if we want to resend. We could refactor to that. To just, close the connections.

  • Ignore my first comment. What do you think about my second comment though? Do we need to call mainPeerConnection.dispose() in closeConnectionAndResend()?

  • Author Maintainer

    Yes, reading on that in the doc. You are right it's used to free up all the resources of that PeerConnection instance. But, I briefly remember that calling dispose from an Observer will cause an error. As discussed here sorry can't link the line. Here is what they are saying about dipose...

    Free native resources associated with this PeerConnection instance.

    This method removes a reference count from the C++ PeerConnection object, which should result in it being destroyed. It also calls equivalent "dispose" methods on the Java objects attached to this PeerConnection (streams, senders, receivers), such that their associated C++ objects will also be destroyed.

    Note that this method cannot be safely called from an observer callback (PeerConnection.Observer, DataChannel.Observer, etc.). If you want to, for example, destroy the PeerConnection after an "ICE failed" callback, you must do this asynchronously (in other words, unwind the stack first). See bug 3721 for more details.

    That's why I called the "close" before the dispose, to unwind the stack. Just making sure it won't cause any error if we place it there. So, I'll give move it there, and test it.

    However, if possible can we finish !4 (closed) first? so that we can rebase this branch with master and use closeConnection in onDestroy as well?

    Edited by HashikD
  • However, if possible can we finish !4 (closed) first? so that we can rebase this branch with master and use closeConnection in onDestroy as well?

    Sure, just merged it.

  • HashikD added 23 commits

    added 23 commits

    • 5b7f0252...70093a2e - 22 commits from branch tpo/anti-censorship/pluggable-transports:master
    • 492e7648 - Moved creation of PeerConnection to offerRequestSuccess

    Compare with previous version

  • HashikD added 1 commit

    added 1 commit

    • 7cac3c5b - Used closeConnections function in onDestroy to clear connections

    Compare with previous version

  • Author Maintainer

    @cohosh It's done. I tested and it's working fine. Is there anything we have to do part of this ticket?

  • Okay this looks good to me.

Please register or sign in to reply
Loading