Add support for socket options

[karpetrosyan]

Closes#662

[karpetrosyan]

This function is very similar to the built-in create_connection function, but it can also accept socket options.
We can't set socket options after a connection has been made because some options affect the connection itself, so we must set them before the connection was made.

[lovelydinosaur]

Okay, I'd suggest we add a comment noting that.

Perhaps rephrased to "This function is equivalent to Python's socket.create_connection() function, but also accepts socket options." and linking to the source on GitHub?

[karpetrosyan]

If we want to support socket options for async interfaces, we need to change a lot of things. Currently, the 'get_asynclib' function is used to get a specific backend interface provided by anyio, then the "connect_tcp" and "connect_unix" asynchronous functions of that module are used to create a connection; how can we override these methods while keeping it as simple as possible?

[karpetrosyan]

As a simple solution, we can associate each library with its function and then call that function.

Old

asyncdefconnect_unix(path: Union[str, "PathLike[str]"]) ->UNIXSocketStream:
path=str(Path(path))
returnawaitget_asynclib().connect_unix(path)

New

asyncdefconnect_unix(path: Union[str, "PathLike[str]"]) ->UNIXSocketStream:
path=str(Path(path))
library=sniffio.current_async_library()
map= {
"trio": trio_connect_tcp,
"asyncio": asyncio_connect_tcp,
}
returnmap[library](path)

[lovelydinosaur]

If we want to support socket options for async interfaces, we need to change a lot of things. Currently, the 'get_asynclib' function is used to get a specific backend interface provided by anyio, then the "connect_tcp" and "connect_unix" asynchronous functions of that module are used to create a connection

It took me a little while to track down what you're referring to here.

how can we override these methods while keeping it as simple as possible?

We'd need to work this through on a case-by-case basis.

The three cases to cover are sync, trio, and asyncio via anyio.

We'd want the PR to start by adapting each of...

• httpcore/backends/sync.py
• httpcore/backends/trio.py
• httpcore/backends/asyncio.py

...and then work through from there.

Let's link to each of these here so it's easier to review how we'd handle this...

httpcore/httpcore/backends/sync.py

Lines 94 to 96 in 82466e1

	sock=socket.create_connection(
	address, timeout, source_address=source_address
	)

httpcore/httpcore/backends/trio.py

Lines 127 to 129 in 82466e1

	stream: trio.abc.Stream=awaittrio.open_tcp_stream(
	host=host, port=port, local_address=local_address
	)

httpcore/httpcore/backends/asyncio.py

Lines 111 to 115 in 82466e1

	stream: anyio.abc.ByteStream=awaitanyio.connect_tcp(
	remote_host=host,
	remote_port=port,
	local_host=local_address,
	)

You've already demonstrated how we'd adapt the sync case.

The trio case would require less duplication to resolve if python-trio/trio#281 was addressed, but otherwise needs their connect_tcp() function to be duplicated.

I think you're pointing out that the asyncio-via-anyio case is actually really awkward to resolve. And yes, that looks correct to me. It does look like duplicating the anyio connect_tcp implementation would be a bit awkward.

🤔

[lovelydinosaur]

Related, and possibly helpful here - it might be worthwhile taking a look at what aiohttp use for their connect_tcp implementation.

[karpetrosyan]

There is an elegant solution, which is to ignore the socket option, which affects the connect method itself.

Our issue is that the socket bind and connect methods behave differently depending on socket options, such as socket.SO_REUSEADDR can help us in connecting to a socket that was previously force closed and is now waiting 1-2 minutes for the socket to be released.

That is very fun, but trio does not support local address binding...

There is also REUSEPORT, which allows you to listen to an IP port pair with multiple sockets, but it is far from httpcore because httpcore is a client library and does not support REUSEPORT at all.

I looked for options that affect bind and connect methods and that httpcore or httpx users can use in their applications, but I couldn't find anything.

I believe it is better to ignore such options and set options after the backend library has been created a connection.
This solution avoids code duplication while keeping code as simple as possible.

This is how the updated function could look.

classBackend(AsyncNetworkBackend):
asyncdefconnect_tcp(
...,
socket_options
):
...
stream=awaitbackend.connect_tcp(
remote_host=host,
remote_port=port,
local_host=local_address,
)
foroptioninsocket_options:
stream._socket.setsockopt(*option)
returnAsyncIOStream(stream)

@tomchristie what are you think about this solution?

[karpetrosyan]

Also, because we don't need options affecting bind or connect, we can use socket.create_connection instead of our newly created one to avoid code duplication

[lovelydinosaur]

Okay, setting socket options after the connect is probably(?) okay for our use-case.

For the async case I'd suggest we push the implementation into httpcore/backends/trio.py and
httpcore/backends/asyncio.py, with httpcore/backends/auto.py calling through to them.

[lovelydinosaur]

Looking good.

Let's also...

• Add this to the CHANGELOG.
• Include an update in the connection pool config docs

[lovelydinosaur]

We should also take a call on if we really do want this, or if what we actually need is just a better default for the sync backed so that it includes TCP_NODELAY.

See #651 (comment)

[karpetrosyan]

We should also take a call on if we really do want this, or if what we actually need is just a better default for the sync backed so that it includes TCP_NODELAY.

See #651 (comment)

I think we should make TCP_NODELAY the default, but also make socket options configurable; there are many options in unix sockets that developers can use, and this can be a powerful feature for developers who know what they're doing.

Additionally, fixing encode/httpx#2635 requires these configurable sockets. which initially inspired me to open this PR :D

[lovelydinosaur]

Suggested change

	self._socket_options=() ifsocket_optionsisNoneelsesocket_options
	self._socket_options=socket_options

it looks to me like we should leave self._socket_options as optional throughout.

[karpetrosyan]

Should we always make sure to check if socket_options is not None before using it?

[lovelydinosaur]

It would make sense for us to do the if socket_options is None dance at the last possible point, in the network backends.

[lovelydinosaur]

Suggested change

	self._socket_options=() ifsocket_optionsisNoneelsesocket_options
	self._socket_options=socket_options

let's keep this optional just now.

[lovelydinosaur]

👍 yes, this is better.

[lovelydinosaur]

let's be consistent with our placement of socket_options in the list, so that the docs match the code.

[lovelydinosaur]

Looks good, yeah.

I've included some possible comments here. (?)