userAgent.close closes all threads that were opened during the call

[paraplexed]

I've had this fix running in our fork for a while now. Fixes #12

[ymartineau]

could you use unix end of lines in RegisterHandler?

if we cancel timers in userAgent.close(), that's probably ok but it means we should create a new UserAgent for next register in peers-gui, etc. and that's not done today. That's okay if you don't have time to do that but it implies an interface change (and all the corresponding testing...).

[paraplexed]

Do you mean running like a dos2unix type command on the file and re-pushing? I'm not sure exactly what you mean by unix end of lines (the github diff only shows me that one if block changed).

I'll definitely test the peers-gui and make the necessary changes for it to work as well.

[ymartineau]

yes, dos2unix on RegisterHandler should be enough to fix end of lines. It
was the same for me, the github diff does not show the end of lines
differences but the local git diff shows lines differences. Thank you

Actually, if you fix peers-gui, could you check all places where
userAgent.close() is called (there are not 50 of them).

thank you

On Wed, Sep 7, 2016 at 12:01 AM, Derek Clifford [email protected]
wrote:

Do you mean running like a dos2unix type command on the file and
re-pushing? I'm not sure exactly what you mean by unix end of lines (the
github diff only shows me that one if block changed).

I'll definitely test the peers-gui and make the necessary changes for it
to work as well.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#24 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABX474aNxnpHLtOSvLjcpKal3jF5Q6D9ks5qneKxgaJpZM4I2iG-
.

[paraplexed]

I ran dos2unix on the RegisterHandler file -- github doesn't seem to like it too much though.

Looking at all the instances of userAgent.close() I found 5 of them. Here's my findings:

• 3 in AccountFrame for the peers-gui
  -- I'm not exactly sure why these are in here in the first place, removing them entirely doesn't seem to make any difference while using the peers-gui, however leaving them in as is does break the gui. The solution I did was to remove them entirely (which I did in my latest commit). Though this means that the TransportManager is not closing the transports like it was previously, and I'm not sure if this is a problem or not (from my testing, I couldn't see any problem)
• 1 in EventManager for the peers-gui
  -- This one is only called when the window for the gui is being called, and I think benefits from the added closures added to the .close method.
• 1 in JsUserAgent for the peers-js
  -- From what I can see, this is never called from within the index.html, so it is a non-concern as it's never used anyways.

Let me know if you have concerns with the removal of userAgent.close in the AccountFrame.

[heruan]

I applied this PR on my local build and when I call UserAgent.terminate(SipRequest) I get caught in this loop when MediaManager.stopSession() is invoked:

MediaManager.java#L281

    public void stopSession() {
        if (rtpSession != null) {
            rtpSession.stop();
            while (!rtpSession.isSocketClosed()) { // <=== socket never closed!
                try {
                    Thread.sleep(15);
                } catch (InterruptedException e) {
                    logger.debug("sleep interrupted");
                }
            }
            rtpSession = null;
        }
        // ...

as if the backing DatagramSocket in RtpSession is never being closed. When/where the socket is supposed to be closed?

This is observable profiling the demo: issuing hangup invokes UserAgent.terminate and the call ends, but the thread gets stuck in the loop above.