Skip to content

Connection - #127

Open
lovelydinosaur wants to merge 5 commits into
mainfrom
connection
Open

lovelydinosaur wants to merge 5 commits into
mainfrom
connection

Conversation

@lovelydinosaur

Copy link
Copy Markdown
Member

The connection class now supports context managed style usage.

Here's how to make a request on a single HTTP connection...

with hip.transports.Connection('https://www.example.com') as c:
    print(c.request('GET', '/'))

Some cleanup still to do around open/closed state.

@lovelydinosaur
lovelydinosaur requested a review from cgl November 2, 2026 15:55

@cgl cgl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good overall. I think there is one issue that needs to be addressed, that is the placeholder Stream() created when no stream is supplied. Could we represent the unopened state without a placeholder stream?

Comment thread tests/test_pool.py
finally:
conn.close()

assert backend.connect_tls_args == ("example.com", 443, "example.com")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def test_context_manager_uses_supplied_stream(server):
    backend = hip.backends.NetworkBackend()
    network_stream = backend.connect(server.host, server.port)

    try:
        with hip.transports.Connection(
            server.url, stream=network_stream
        ) as connection:
            assert connection._stream is network_stream
            assert connection.request("GET", "/").status_code == 200

        assert network_stream._is_closed
    finally:
        network_stream.close()

Can we add a regression test like this? open() creates a new connection even when a stream was supplied.

Comment thread src/hip/_connection.py
self._stream = backend.connect_tls(host, port)
else:
self._stream = backend.connect(host, port)
self._parser = HTTPParser(self._stream, mode="CLIENT")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This repeats the parser initialisation in the constructor. Maybe we can postpone the parser creation until after connection is opened.

Comment thread src/hip/_connection.py
Comment on lines +100 to +103
if scheme == "https":
self._stream = backend.connect_tls(host, port)
else:
self._stream = backend.connect(host, port)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines works ok well when no stream is supplied, but otherwise they create a second connection and overwrite self._stream, leaving the supplied stream open.

@lovelydinosaur

lovelydinosaur commented Nov 4, 2026 •

Copy link
Copy Markdown
Member Author

Thanks for the review. Yeah there's a bunch of extra state cleanup this'd need.

Informally, this is motivated by wanting to have a consistent stack all the way up, so...

# Working directly at the stream level
backend = hip.NetworkBackend()
with backend.connect(host="187.54.30.60", port=80) as s:
    s.write(
        b"GET / HTTP/1.1\r\n"
        b"Host: example.com\r\n"
        b"Connection: close\r\n"
        b"\r\n"
    )
    s.read()

# Integrating an HTTP parser...
backend = hip.NetworkBackend()
with backend.connect(host="187.54.30.60", port=80) as s:
    p = hip.HTTPParser(mode=hip.CLIENT)
    p.send_method_line(b“GET”, b“/“)
    p.send_headers([(b"Host", b"www.example.com")])
    p.send_body(b"")
    p.recv_status_line()
    p.recv_headers()
    p.recv_body()

# Integrating a request/response API...
with hip.Connection("https://www.example.com") as conn:
    conn.request("GET", "/")

# Supporting multiple connections...
with hip.ConnectionPool() as pool:
    pool.request("GET", "https://www.example.com")

# Adding support for redirects, cookie handling & content parsing...
with hip.Client() as cli:
    cli.get("https://www.example.com")

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants