Opened 3 months ago

Last modified 3 days ago

#26389 reopened enhancement

Remove `handlerChan`, shut down immediately on SIGTERM

Reported by: cypherpunks Owned by: dcf
Priority: Low Milestone:
Component: Obfuscation/meek Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description


Child Tickets

Change History (7)

comment:1 Changed 3 months ago by cypherpunks

After Update client shutdown procedure commit meek-client keep running roundTripRetries after shutdown signal.
Reason it work before:

-	if sig == syscall.SIGTERM {
-		log.Printf("done")
-		return
-	}

Solution. To replace:

time.Sleep(retryDelay)

By something like this:

			select {
			case <-shutdownChan:
				return resp, err
			case <-time.After(retryDelay):
				break
			}

Then to inform it on exit, by something like this:

	close(shutdownChan)
	for _, ln := range listeners {
		ln.Close()
	}
Last edited 3 months ago by cypherpunks (previous) (diff)

comment:2 Changed 3 months ago by cypherpunks

A select blocks until one of its cases can run, then it executes that case. It chooses one at random if multiple are ready.

Races, races, races.
Solution is not solution.

comment:3 Changed 3 months ago by cypherpunks

Resolution: invalid
Status: newclosed

meek is deprecated.

comment:4 in reply to:  3 Changed 3 months ago by gk

Resolution: invalid
Status: closedreopened

Replying to cypherpunks:

meek is deprecated.

I don't think so.

comment:5 Changed 2 months ago by dcf

Priority: MediumLow

So, I think this is actually intentional behavior, though now that I look at it, it's conforming to an older version of pt-spec.txt and doing something that's no longer required.

This is what the spec said from 2012 to 2014.
https://gitweb.torproject.org/torspec.git/tree/pt-spec.txt?id=ffad9203dfda7848d780bf258dd141a2ea42e16f#n348

Proxies should respond to a single INT signal by closing their
listener ports and not accepting any new connections, but keeping
all connections open, then terminating when connections are all
closed. Proxies should respond to a second INT signal by shutting
down cleanly.

Keeping track of open connections was the whole purpose of handlerChan and the loop at the end of main that waits until all handlers are done.

But the require to keep existing connections open was removed in 2014. This is what the current version says:
https://gitweb.torproject.org/torspec.git/tree/pt-spec.txt?id=86480728d816474a0771a3b3aba5d223a32f0705#n620

PT proxies SHOULD handle OS specific mechanisms to gracefully
terminate (eg: Install a signal handler on 'SIGTERM' that
causes cleanup and a graceful shutdown if able).

Now it only says that we have to do a graceful shutdown, which to me doesn't imply that we have to keep existing connections open. So we could just shut everything down, which seems to be what the ticket reporter expected anyway.

The way I would recommend to do this would be to remove all the code having to do with handlerChan. When main returns, it will terminate the goroutine running roundTripRetries, I think. cypherpunks, do you want to test that? Otherwise, I don't think the current behavior is causing much of a problem, so I'll mark it as a low priority.

comment:6 Changed 2 months ago by dcf

Oh, but there actually was a separate bug: #24875. That one was about meek-client never terminating in the case when it received a signal but had zero handlers running. That's been fixed, but the fix hasn't yet appeared in a release.

comment:7 Changed 3 days ago by dcf

Summary: meek-client keep roundTripRetries on shutdownRemove `handlerChan`, shut down immediately on SIGTERM
Type: defectenhancement
Note: See TracTickets for help on using tickets.