Do we want to close all connections when tor closes?

I have a branch for privcount that closes all connections before tor shuts down. Is this a feature that we want?

changed milestone to %Tor: unspecified

added 034-included20180401 034-triage-20180328 035-deferred-20190115 035-roadmap-subtask 035-triaged-in-20180711 041-proposed chutney-wants component::core tor/tor correctness milestone::Tor: unspecified parent::25510 points::0.5 priority::high privcount review-group-24 severity::normal shutdown status::needs-revision type::enhancement labels

Trac:
Owner: N/A to teor
Status: new to assigned

My ticket23573 branch has some draft code that implements this issue. It also has the sleep(1); that mitigates #23570 (moved).

If we want to add these checks that our connection shutdown code works, I can revise it and write a changes file. It would be useful for chutney, which doesn't rotate connections in the short time it's running a network.

Or we can just let the OS do the connection cleanup for us.

Trac:
Status: assigned to needs_review
Keywords: N/A deleted, chutney-wants, correctness added

review-group-24 is now open.

Trac:
Keywords: N/A deleted, review-group-24 added

Calling this sponsor8 because of #23847 (moved)

Trac:
Sponsor: N/A to Sponsor8

Needs a changes file.

I'd rather have a real fix for #23570 (moved) than the sleep() call here, if at all possible. Maybe we should just fflush() everything?

I think that rather than having main.c call hibernate_go_dormant, we should extract the relevant part of hibernate_go_dormant into a new function, and have main.c call that.

Is the connection_mark_for_close() in hibernate_go_dormant enough here? It only marks the connections; it doesn't necessarily close them.

The commit message says Implements #435, but I don't think that's right?

Trac:
Status: needs_review to needs_revision

Is the connection_mark_for_close() in hibernate_go_dormant enough here? It only marks the connections; it doesn't necessarily close them.

Oh! Does this patch assume that #23571 (moved) is also merged?

Replying to nickm:

Needs a changes file.

Ack.

I'd rather have a real fix for #23570 (moved) than the sleep() call here, if at all possible. Maybe we should just fflush() everything?

The sleep() call isn't a great fix. I think fflush() would be a good idea: I'm not sure if it's enough.

I think that rather than having main.c call hibernate_go_dormant, we should extract the relevant part of hibernate_go_dormant into a new function, and have main.c call that.

Ack.

The commit message says Implements #435, but I don't think that's right?

It's right for experimental PrivCount :-) I'll fix it now I know we want something like this upstreamed.

Replying to nickm:

Is the connection_mark_for_close() in hibernate_go_dormant enough here? It only marks the connections; it doesn't necessarily close them.

Oh! Does this patch assume that #23571 (moved) is also merged?

Yes. Or at least, it doesn't try to duplicate that functionality.

Replying to teor:

Replying to nickm:

Needs a changes file.

Ack.

I'd rather have a real fix for #23570 (moved) than the sleep() call here, if at all possible. Maybe we should just fflush() everything?

The sleep() call isn't a great fix. I think fflush() would be a good idea: I'm not sure if it's enough.

The correct call here is queued_events_flush_all(1). The sleep(1) does nothing (or doesn't do much), because we've just shut down all our timers. Also, the sleep() call crashes tor, if the sandbox is enabled.

Edit: word order

Moving most of my assigned tickets to 0.3.4

Trac:
Milestone: Tor: 0.3.3.x-final to Tor: 0.3.4.x-final

Trac:
Keywords: N/A deleted, 034-triage-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

Trac:
Keywords: N/A deleted, 034-removed-20180328 added

This needs to happen (or something like it!) as part of embedding.

Trac:
Parent: N/A to #25510 (moved)
Keywords: 034-removed-20180328 deleted, 034-included20180401 added

Unassigning myself, because someone might want to do this task before I have time.

Trac:
Owner: teor to N/A
Status: needs_revision to assigned

The required revision is: replace sleep() with queued_events_flush_all(1)

https://trac.torproject.org/projects/tor/ticket/23573#comment:8

Trac:
Status: assigned to needs_revision

And some refactoring, and actually closing the connections, not just marking them: https://trac.torproject.org/projects/tor/ticket/23573#comment:5

Move most needs_revision tickets from 0.3.4 to 0.3.5: we're about to hit the feature freeze.

If you really need to get this into 0.3.4, please drop me a line. :)

Trac:
Milestone: Tor: 0.3.4.x-final to Tor: 0.3.5.x-final

Trac:
Keywords: N/A deleted, 035-roadmap-subtask added

Trac:
Keywords: N/A deleted, 035-triaged-in-20180711 added

Trac:
Priority: Medium to High

Trac:
Sponsor: Sponsor8 to Sponsor19

I don't think this is really sponsor-19.

Trac:
Sponsor: Sponsor19 to N/A

Marking a number of 0.3.5 tickets as possible, maybe even a good idea, for later. Possibly backportable, some of them. But not currently things to do as part of 0.3.5 stabilization.

Trac:
Milestone: Tor: 0.3.5.x-final to Tor: unspecified
Keywords: N/A deleted, 041-proposed, 035-deferred-20190115 added

changed time estimate to 4h

moved to tpo/core/tor#23573 (closed)