Opened 19 months ago

Last modified 3 months ago

#23573 needs_revision enhancement

Do we want to close all connections when tor closes?

Reported by: teor Owned by:
Priority: High Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: shutdown, privcount, correctness, chutney-wants, review-group-24, 034-triage-20180328, 034-included20180401, 035-roadmap-subtask, 035-triaged-in-20180711, 035-deferred-20190115, 041-proposed
Cc: Actual Points:
Parent ID: #25510 Points: 0.5
Reviewer: Sponsor:

Description

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

Child Tickets

Change History (22)

comment:1 Changed 19 months ago by teor

Owner: set to teor
Status: newassigned

comment:2 Changed 19 months ago by teor

Keywords: correctness chutney-wants added
Status: assignedneeds_review

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

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.

Last edited 19 months ago by teor (previous) (diff)

comment:3 Changed 18 months ago by nickm

Keywords: review-group-24 added

review-group-24 is now open.

comment:4 Changed 18 months ago by nickm

Sponsor: Sponsor8

Calling this sponsor8 because of #23847

comment:5 Changed 18 months ago by nickm

Status: needs_reviewneeds_revision

Needs a changes file.

I'd rather have a real fix for #23570 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?

comment:6 Changed 18 months ago by 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 is also merged?

comment:7 in reply to:  6 ; Changed 18 months ago by teor

Replying to nickm:

Needs a changes file.

Ack.

I'd rather have a real fix for #23570 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 is also merged?

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

comment:8 in reply to:  7 Changed 15 months ago by teor

Replying to teor:

Replying to nickm:

Needs a changes file.

Ack.

I'd rather have a real fix for #23570 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 the sandbox, if it is enabled.

Version 0, edited 15 months ago by teor (next)

comment:9 Changed 15 months ago by teor

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

Moving most of my assigned tickets to 0.3.4

comment:10 Changed 13 months ago by nickm

Keywords: 034-triage-20180328 added

comment:11 Changed 13 months ago by nickm

Keywords: 034-removed-20180328 added

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

comment:12 Changed 13 months ago by nickm

Keywords: 034-included20180401 added; 034-removed-20180328 removed
Parent ID: #25510

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

comment:13 Changed 13 months ago by teor

Owner: teor deleted
Status: needs_revisionassigned

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

comment:14 Changed 13 months ago by teor

Status: assignedneeds_revision

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

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

comment:15 Changed 13 months ago by teor

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

comment:16 Changed 11 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: 0.3.5.x-final

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. :)

comment:17 Changed 10 months ago by nickm

Keywords: 035-roadmap-subtask added

comment:18 Changed 10 months ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:19 Changed 7 months ago by nickm

Priority: MediumHigh

comment:20 Changed 3 months ago by gaba

Sponsor: Sponsor8Sponsor19

comment:21 Changed 3 months ago by nickm

Sponsor: Sponsor19

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

comment:22 Changed 3 months ago by nickm

Keywords: 035-deferred-20190115 041-proposed added
Milestone: Tor: 0.3.5.x-finalTor: unspecified

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.

Note: See TracTickets for help on using tickets.