[PATCH stable] commandserver: handle backlog before exiting

[Yuya Nishihara]

On Tue, 31 Jan 2017 20:11:09 +0000, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2017-02-01 00:48:26 +0900:
> > On Tue, 31 Jan 2017 15:24:42 +0000, Jun Wu wrote:
> > > Excerpts from Yuya Nishihara's message of 2017-01-31 23:36:46 +0900:
> > > > > This introduces another race condition that unlinks an innocent socket file.
> > > > > I will send a V2 later.
> > > 
> > > (This actually seems to be a regression after the socketserver refactoring.
> > > It probably requires sub-classing "unixforkingservice" in chg to solve
> > > properly. It's not too trivial and I'll send fixes after the freeze.)
> > 
> > I don't think SocketServer does anything special for flushing backlog.
> 
> Sorry, I missed chgunixservicehandler's customized "unlinksocket"
> implementation. So chg should actually work just fine, while commandserver
> needs some care to avoid double unlink (which is probably just adding an
> "if" to prevent the second unlink).

Ok, I think I got your idea, which is:

 on successful exit:
 1. unlink(sockpath) for pseudo closing
 2. accept() and fork() for each queued connection
 3. real close() at end
 # we have to avoid double unlink()s here

 on failure:
 0. someone raises exception
 1or2. unlink(sockpath)
 2or1. close()

That sounds good.

> > > I think the "industry standard" about shutting down is to handle all
> > > remaining requests while rejecting new ones. The pattern is seen in nginx,
> > > squid, thin (a Ruby on Rails server), OpenStack APIs etc.
> > 
> > If they do, we can copy. Can you investigate how they solve the problem?
> > I tried if shutdown(SHUT_RD) could be used, but it didn't help as expected.
> 
> The epoll server (thin / eventmachine) uses "epoll_ctl (epfd, EPOLL_CTL_DEL,
> ...)". Squid code seems too complex. I guess "shutdown" is for connections
> that are the return value of accept (3), while we want to deal with the
> "socket" argument of accept (3), so shutdown does not help.

I guess epoll_ctl() would just stop event emission and the kernel would
continue queuing new connection. Anyway, we can do unlink(sockpath) as you
said, so complex hack wouldn't be needed. Thanks for checking the real-world
implementation.

OT: typical use of shutdown() I know is to send FIN to close the read end
at the server side.

 client: shutdown(SHUT_WR)
 server: receives FIN and recv() ends with EOF
 server: sends response to client
 client: receives response from server
 client: close() the session

We could do shutdown(SHUT_RD) for listening socket, but which just prevented
us from calling accept()s. I just tried that in case there's some trick I
didn't know.

> > > It looks better
> > > than retrying in the client.
> > 
> > Yes, but we have retry for connect(), so adding retry for the first recv()
> > wouldn't make things much worse.
> 
> That's actually interesting as it can probably cover ECONNREFUSED too. I'll
> try if it could be implemented cleanly. That said, I would still like the
> server-side fix which is simply effective.

Agreed. Server-side fix would be cleaner.