fix: unix socket support multi workers

[Hanaasagi]

• resolve UNIX domain socket raises 503 when workers are used #428 .
• fix None remote addr in access log.

[tomchristie]

Curious: Why do we remove the UDS socket here?

[Hanaasagi]

From man 7 Unix

Binding to a socket with a filename creates a socket in the filesystem that must be deleted by the caller when it is no longer needed (using unlink(2)). The usual UNIX close-behind semantics apply; the socket can be unlinked at any time and will be finally removed from the filesystem when the last reference to it is closed.

I think there is another problem. If our process crashes, os.remove() will not be called and we’ll have a dangling socket. It seems that we need to check the socket file before bind().

---

Why haven’t encountered this problem when use one worker and unix socket mode?

Because we use the create_unix_server

uvicorn/uvicorn/main.py

Lines 363 to 373 in e0a77de

	elif config.uds is not None:
	# Create a socket using UNIX domain socket.
	uds_perms = 0o666
	if os.path.exists(config.uds):
	uds_perms = os.stat(config.uds).st_mode
	server = await loop.create_unix_server(create_protocol, path=config.uds)
	os.chmod(config.uds, uds_perms)
	message = "Uvicorn running on unix socket %s (Press CTRL+C to quit)"
	self.logger.info(message % config.uds)
	self.servers = [server]

create_unix_server will unlink the socket file before it starts.

https://github.com/python/cpython/blob/0d3fe8ae4961bf551e7d5e42559e2ede1a08fd7c/Lib/asyncio/unix_events.py#L283-L293

[tomchristie]

Is there a reason why we're changing the None case here?

[Hanaasagi]

None was displayed in access log.

2019-10-08 15:47:52,870 - INFO - None - "GET / HTTP/1.1" 200
2019-10-08 15:48:00,546 - INFO - None - "GET / HTTP/1.1" 200
2019-10-08 15:48:01,285 - INFO - None - "GET / HTTP/1.1" 200

Or we can fix here

diff --git a/uvicorn/protocols/http/httptools_impl.py b/uvicorn/protocols/http/httptools_impl.py
index 2e1dd90..8b414b2 100644
--- a/uvicorn/protocols/http/httptools_impl.py
+++ b/uvicorn/protocols/http/httptools_impl.py
@@ -436,7 +436,7 @@ class RequestResponseCycle:
             if self.access_log:
                 self.logger.info(
                     '%s - "%s %s HTTP/%s" %d',
-                    self.scope["client"],
+                    self.scope["client"] or "-",
                     self.scope["method"],
                     get_path_with_query_string(self.scope),
                     self.scope["http_version"],

Any good idea, thanks?

[jcwilson]

I just bumped into this today, as well. I can use TCP for now, but would love to help out here if I can.

[gisthere]

really needed

[Hanaasagi]

Emm, I will fix the conflicts recently.

[Roang-zero1]

Both the issue I had #586 and the Issue #722 could also be solved by using the fix proposed in this pull request.
By moving the check to config.bind_socket as proposed in #722 any further additions to watcher types would also be agnostic to the type of socket used.
I am willing to work on refactoring as needed.

[Kludex]

I'll close this PR, as #1009 addressed the issue that motivated it.