Opened 14 months ago

Last modified 7 weeks ago

#23573 needs_revision enhancement

Do we want to close all connections when tor closes?

Reported by: teor Owned by:
Priority: High Milestone: Tor: 0.3.5.x-final
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
Cc: Actual Points:
Parent ID: #25510 Points: 0.5
Reviewer: Sponsor: Sponsor8

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 (19)

comment:1 Changed 14 months ago by teor

Owner: set to teor
Status: newassigned

comment:2 Changed 14 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 14 months ago by teor (previous) (diff)

comment:3 Changed 13 months ago by nickm

Keywords: review-group-24 added

review-group-24 is now open.

comment:4 Changed 13 months ago by nickm

Sponsor: Sponsor8

Calling this sponsor8 because of #23847

comment:5 Changed 13 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 13 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 13 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 10 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 tor, if the sandbox is enabled.

Edit: word order

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

comment:9 Changed 10 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 8 months ago by nickm

Keywords: 034-triage-20180328 added

comment:11 Changed 8 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 8 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 8 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 8 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 8 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 6 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 5 months ago by nickm

Keywords: 035-roadmap-subtask added

comment:18 Changed 4 months ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:19 Changed 7 weeks ago by nickm

Priority: MediumHigh
Note: See TracTickets for help on using tickets.