Closed Bug 962358 Opened 9 years ago Closed 7 years ago

Provide an API/observer to close persistent connections

Categories

(Core :: Networking, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: mikeperry, Assigned: huseby)

References

(Blocks 2 open bugs)

Details

(Whiteboard: lame-network [tor])

Attachments

(2 files)

In Tor Browser, we make it easy for users to clear browser state and get a new Tor circuit. As part of this process, we want to shut down existing HTTP (and also SPDY, but we currently disable it) keepalive connections. We created an observer topic for this process:
https://gitweb.torproject.org/tor-browser.git/commitdiff/99dbd2c48a9e8824bf69a0879daf07e6d5e2683c

Is there a better way to do this? Or is an observer topic acceptable?
Component: General → Networking
Product: Firefox → Core
independent of tor, we're going to need something like this to deal with topology changes (and some pref changes in psm).. the patch basically matches what I want to do.
Whiteboard: lame-network
Whiteboard: lame-network → lame-network [tor]
Should be similar to #939318 then I take it. React on network (topology) changes.
(In reply to Daniel Stenberg [:bagder] from comment #2)
> Should be similar to #939318 then I take it. React on network (topology)
> changes.

I am confused what you mean here. It looks like you have an observer topic that gets emitted on network change. Do you want my observer to get fired when yours fires? Or do you just want my observer to listen for the same topic and state value as your code from bug 939319, instead of the new "net:prune-all-connections" one I created?

Here's a valid commit url with my original patch (the old URL had its hash invalidated by the Mozilla git migration a month or so back):
https://gitweb.torproject.org/tor-browser.git/commitdiff/51d758b204ab45f75802a48be452d35db32e95da
Assignee: nobody → huseby
Mike, it looks like the link to your patch is invalid again.  Can you find it again for me and post it here?  Thanks!
Flags: needinfo?(mikeperry)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
This adds a net:prune-all-connections message handler to nsHttpHandler.  This is used by TorButton in the Tor Browser to force close existing connections.
Attachment #8661606 - Flags: review?(mcmanus)
Attachment #8661606 - Flags: review?(arthuredelstein)
This is the unit test for prune all connections but there is a race condition.  It only works when debugging output is enabled.  The problem is that the net:prune-all-connections only gets processed if it fires after the channel is established and the connection is accepted but *before* the HTTP request is initialized.  I'm not sure how to get a callback to fire after the channel is set up but before the HTTP request is initialized.  The listener passed to channel.asyncOpen never has any of its callbacks called.
Attachment #8661608 - Flags: review?(mcmanus)
Also, if the net:prune-all-connections is sent after the HTTP request is made on the client side, it appears that the nsHttpHandler doesn't ever receive the messages.  The nsHttpHandler::Observe function doesn't get called.  Does this need to pump the i/o loop or kick the i/o thread or something?
first, apologies for the lag getting to this. It wasn't obvious what was going on without digging around. I still haven't set it up - so I'll guess:

it looks like on the server side, you go to do a read (onInputStreamReady) and if you get an EOF (via an exception) you declare the success but if you get data (i.e. and http request) you get a failure. And there is a race condition in between those. Am I reading that right?

I would suggest that you implement an output handler that sends out
http/1.1 200 ok
connection: keep-alive
content-length: 0
\n

and then see if the client closes the connection at any point in the next mumble~10 seconds. (including the case where it does it now without sending any data). Firefox would normally wait 2+ minutes to close that socket, but the prune-connections observable is obviously meant to short circuit that.
Comment on attachment 8661608 [details] [diff] [review]
Bug_962358_test.patch misbehaving unit test

Review of attachment 8661608 [details] [diff] [review]:
-----------------------------------------------------------------

I think we can fix this race
Attachment #8661608 - Flags: review?(mcmanus)
Attachment #8661606 - Flags: review?(mcmanus) → review+
Comment on attachment 8661606 [details] [diff] [review]
Bug_962358.patch adds net:prune-all-connections system message for TorButton

Review of attachment 8661606 [details] [diff] [review]:
-----------------------------------------------------------------

The main patch looks OK to me. I do think a unit test for this will be useful at some point, to ensure that there are no lingering connections.
Attachment #8661606 - Flags: review?(arthuredelstein) → review+
the try looks clean to me.
Keywords: checkin-needed
DO NOT check in the unit test, just the main patch.
https://hg.mozilla.org/mozilla-central/rev/7d9434a0044f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: meta_tor
You need to log in before you can comment on or make changes to this bug.