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. _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
