softprops / hyperlocal

🔌 ✨rustlang hyper bindings for local unix domain sockets
MIT License
227 stars 46 forks source link

Cargo release 0.9.0 #66

Closed fussybeaver closed 1 week ago

fussybeaver commented 6 months ago

Happy to see this crate integrate with the latest Hyper release in https://github.com/softprops/hyperlocal/pull/65 . This is working well with my testing. Is there any remaining work to publish to crates.io ?

Would be keen to see a 0.9.0 release. Grateful for any time spent on this.

@softprops

jalaziz commented 4 months ago

Just poking here in hopes this simply got buried.

I'd be happy to help with maintenance or anything needed to get a new release out.

softprops commented 4 months ago

Hi there.

I'm testing this and when I run the server and then the client I get Failed to accept connection: hyper::Error(Shutdown, Os { code: 57, kind: NotConnected, message: "Socket is not connected" }).

any ideas @iamjpotts ?

softprops commented 4 months ago

Note I do get a listening server and a response on the client. I just didn't understand the semantics of the error reported, if it is an error or not ect

iamjpotts commented 4 months ago

Hi there.

I'm testing this and when I run the server and then the client I get Failed to accept connection: hyper::Error(Shutdown, Os { code: 57, kind: NotConnected, message: "Socket is not connected" }).

any ideas @iamjpotts ?

What operating system?

Can you provide a minimum repro code snippet?

softprops commented 4 months ago

What operating system?

osx

rustc --version     
rustc 1.76.0 (07dca489a 2024-02-04)

Can you provide a minimum repro code snippet?

this is me running the examples in that live in the repo from the usage docs https://github.com/softprops/hyperlocal?tab=readme-ov-file#usage

iamjpotts commented 4 months ago

I am able to reproduce the issue on osx 14 (Sonoma) but not Ubuntu 20.04.

cargo run --features server --example server in one terminal and then cargo run --example client in another terminal. Server outputs the error in your comment once for every client run.

iamjpotts commented 4 months ago

If in the example client I add a tokio::time::sleep for two seconds after the while loop and before the Ok(()) then the same error message appears, but it is delayed by two seconds, and appears on the server when the client exits.

There are very few references to Kind::Shutdown in the hyper code base, and the server code that results in the Shutdown Err is called by https://github.com/hyperium/hyper/blob/0013bdda5cd34ed6fca089eceb0133395b7be041/src/proto/h1/dispatch.rs#L135-L136 which is calling trait method Dispatch::recv_msg which has two implementations - one for a client, and one for a server. The server implementation simply returns the Err(e) it is passed, which is what the call site in the hyperlocal server sample receives as hyper::Error(Shutdown...). Unlike the server, the client implementation of recv_msg has several things it tries before returning Err.

For the hyperlocal server example, I am inclined to treat a hyper::Error(Shutdown) response as a success response. Does that seem sensible here @softprops ?

If not, I may be at the limit of my knowledge of hyper for how to resolve this.

iamjpotts commented 4 months ago

https://github.com/softprops/hyperlocal/pull/67 treats the disconnect as a success, but I am not sure this is the proper approach.

It turns out that hyper does not expose a way to detect Kind::Shutdown and that PR had to resort to inspecting an inner cause error. It seems suspicious that serve_connection doesn't return on OSX until the client has disconnected.

iamjpotts commented 4 months ago

@fussybeaver what are your thoughts on the behavior of serve_connection on OSX in the updated server example?

fussybeaver commented 4 months ago

I can reproduce this behaviour on an OSX machine. If I add half_close(true) to the server builder, the connection will be made on OSX though, so perhaps this is just and idiosyncratic behaviour of Tokio's UnixStream implementation on OSX.

https://docs.rs/hyper/latest/hyper/server/conn/http1/struct.Builder.html#method.half_close

iamjpotts commented 4 months ago

A similar issue was reported in https://github.com/softprops/hyperlocal/issues/61 for version 0.8.0 (the currently published version of the crate that is several years old).

I suspect, though waiting for confirmation, that the individual who reported the issue in #61 is also on OSX.

This seems to be a behavior of OSX and some other operating systems (excluding Linux and Windows) - that if the client closes the connection, the server will get an ENOTCONN error when trying to shutdown the connection.

A very old Node.js PR mentions this: https://github.com/nodejs/node/commit/d5b32246fb

The ENOTCONN error is coming from this call in std's Unix socket shutdown() call:

cvt(unsafe { libc::shutdown(self.as_raw_fd(), how) })?;

Basically poll_shutdown and related variants pass thru the error returned by the operating system's implementation of libc::shutdown - which on OSX, is an error ENOTCONN if the peer is shutdown.

iamjpotts commented 4 months ago

@fussybeaver I did not see any change in behavior on OSX on the server when setting half_close(true) - it was still getting an Err back from serve_connection at the moment the client connection is dropped (closed), as evidenced by Client disconnected in the server output.

When you set half_close(true) on the server in the latest version of main are you getting Client disconnected or Accepted connection (to be distinguished from Accepting connection)?

fussybeaver commented 3 months ago

@iamjpotts I realise after adjusting the println statements that it doesn't work with half_close(true), but I do get it to work when we disable keep_alive on the server.

There's a comment in the hyper_util codebase around keep alive and unsafe raw file pointer handling: https://github.com/hyperium/hyper-util/blob/61724d117163adf2195c701fa8e06f5c22c0a64d/src/client/legacy/connect/http.rs#L680-L682

kanpov commented 2 weeks ago

I'm currently working on a crate named hyper-client-sockets, that will similar to hyperlocal but will fully support hyper v1 and focuses only on clientside hyper (since serverside is irrelevant since hyper v1) with the support for unix, vsock and firecracker sockets (under features). It'll probably be available on crates.io soon.

Update: https://crates.io/crates/hyper-client-sockets/0.1.0

fussybeaver commented 1 week ago

I could get behind that.. or if @iamjpotts takes over the regular maintenance/release of this crate, since it works fine.

A stable and compatible connection pooling implementation would be quite useful, as hyper_util's one has issues...

kanpov commented 1 week ago

I could get behind that.. or if @iamjpotts takes over the regular maintenance/release of this crate, since it works fine.

A stable and compatible connection pooling implementation would be quite useful, as hyper_util's one has issues...

Interesting. What are the issues with hyper_util's Client?

kanpov commented 1 week ago

I could get behind that.. or if @iamjpotts takes over the regular maintenance/release of this crate, since it works fine.

A stable and compatible connection pooling implementation would be quite useful, as hyper_util's one has issues...

Also, I don't see why the server part of hyperlocal shouldn't be removed or at least deprecated. As far as I've seen, it's trivial to bind to unix instead of tcp in hyper v1

fussybeaver commented 1 week ago

Interesting. What are the issues with hyper_util's Client?

There is/(was?) a race condition in the Hyper connection pool that was carried over from the pre-1.x Hyper versions, possibly there's some movement to fix it lately, but as yet not sure it works, as documented here: https://github.com/hyperium/hyper/issues/2312

As to your later question, I think the crate is mainly on life support and needs a revival / new maintainer

iamjpotts commented 1 week ago

I do not have any particular affinity to hyperlocal, and probably would not be interested in being a maintainer.

I thought it would be good to keep maintaining an existing crate if the current maintainer wants to continue shepherding it and publishing releases. It seems ready for a 0.9.0 release now.

If hyperlocal fell out of maintenance and other crate became actively maintained, I would probably switch to the other crate.

softprops commented 1 week ago

Hi folks, sorry for the delay. I just pushed up 0.9.0 to crates.io.