962358 - Provide an API/observer to close persistent connections

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Mike Perry]

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?

Comment 1

[Patrick McManus [:mcmanus]]

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.

Comment 2

[Daniel Stenberg [:bagder]]

Should be similar to #939318 then I take it. React on network (topology) changes.

Comment 3

[Mike Perry]

(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

Comment 4

[Dave Huseby [: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!

Comment 5

[Arthur Edelstein [:arthur]]

Here's the latest version:
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-38.1.0esr-5.0-1&id=e8fa092280b57f8315115f25bc4d18a909bae8a8

Comment 6

[Dave Huseby [:huseby]]

Attachment: Bug_962358.patch adds net:prune-all-connections system message for TorButton

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.

Comment 7

[Dave Huseby [:huseby]]

Attachment: Bug_962358_test.patch misbehaving unit test

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.

Comment 8

[Dave Huseby [:huseby]]

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?

Comment 9

[Patrick McManus [:mcmanus]]

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 10

[Patrick McManus [:mcmanus]]

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

Comment 11

[Arthur Edelstein [:arthur]]

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.

Comment 12

[Dave Huseby [:huseby]]

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6b45a072363

Comment 13

[Dave Huseby [:huseby]]

the try looks clean to me.

Comment 14

[Dave Huseby [:huseby]]

DO NOT check in the unit test, just the main patch.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d9434a0044f

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7d9434a0044f