Opened 2 years ago

Closed 19 months ago

#26389 closed enhancement (fixed)

Remove `handlerChan`, shut down immediately on SIGTERM

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


Child Tickets

Change History (8)

comment:1 Changed 2 years 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:


By something like this:

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

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

	for _, ln := range listeners {
Last edited 2 years ago by cypherpunks (previous) (diff)

comment:2 Changed 2 years 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 2 years ago by cypherpunks

Resolution: invalid
Status: newclosed

meek is deprecated.

comment:4 in reply to:  3 Changed 2 years ago by gk

Resolution: invalid
Status: closedreopened

Replying to cypherpunks:

meek is deprecated.

I don't think so.

comment:5 Changed 2 years 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.

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:

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 years 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 23 months ago by dcf

Summary: meek-client keep roundTripRetries on shutdownRemove `handlerChan`, shut down immediately on SIGTERM
Type: defectenhancement

comment:8 Changed 19 months ago by dcf

Keywords: goptlib added
Resolution: fixed
Status: reopenedclosed
Note: See TracTickets for help on using tickets.