Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cargo release 0.9.0 #66

Closed
fussybeaver opened this issue Jan 20, 2024 · 20 comments
Closed

Cargo release 0.9.0 #66

fussybeaver opened this issue Jan 20, 2024 · 20 comments

Comments

@fussybeaver
Copy link

Happy to see this crate integrate with the latest Hyper release in #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
Copy link

jalaziz commented Mar 7, 2024

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

#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
Copy link
Contributor

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

@fussybeaver
Copy link
Author

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
Copy link
Contributor

A similar issue was reported in #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: nodejs/node@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
Copy link
Contributor

@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
Copy link
Author

@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
Copy link

kanpov commented Jul 17, 2024

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
Copy link
Author

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
Copy link

kanpov commented Jul 21, 2024

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
Copy link

kanpov commented Jul 22, 2024

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
Copy link
Author

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: hyperium/hyper#2312

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

@iamjpotts
Copy link
Contributor

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
Copy link
Owner

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

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

No branches or pull requests

5 participants