Skip to content
This repository has been archived by the owner on May 1, 2021. It is now read-only.

switch to socket2 #5

Merged
merged 3 commits into from
Feb 14, 2021
Merged

switch to socket2 #5

merged 3 commits into from
Feb 14, 2021

Conversation

Keruspe
Copy link
Contributor

@Keruspe Keruspe commented Jan 11, 2021

Now that socket2 is getting the attention it needed, this simplifies a lot the code, which just becomes a thin wrapper.

This also fixes #1 as a side-effect.

@Keruspe
Copy link
Contributor Author

Keruspe commented Jan 11, 2021

(I actually wanted to try this out to fix #1 in the first place. I'm not sure if we want to do that, but since socket2 might eventually become the std impl, I though this could make sense?)

This will oviously have to wait for 0.4 to actually be released

Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
required by socket2 for MaybeUninit to impl Debug
Required by socket2 for usize::MAX
required by socket2 for match in const fn

Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
@taiki-e
Copy link
Collaborator

taiki-e commented Jan 11, 2021

This will be blocked until the 0.4 release of socket2, but I'm in favor of this. (It is more actively maintained/developed than nb-connect and has been checked/tested on various platforms.)

(The socket2 maintainer previously said that socket2 is not ready for production use, but that will probably improve in 0.4.)

EDIT: marked this PR as draft as this is blocked until the 0.4 release of socket2.

@taiki-e taiki-e marked this pull request as draft January 11, 2021 14:47
@taiki-e taiki-e mentioned this pull request Jan 11, 2021
Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
@Keruspe
Copy link
Contributor Author

Keruspe commented Jan 14, 2021

@taiki-e I guess downgrading to 0.3 could unblock this? Then we'll just be able to revert this to switch to 0.4 once released

@Keruspe Keruspe marked this pull request as ready for review January 15, 2021 10:59
@JayceFayne
Copy link

JayceFayne commented Jan 15, 2021

Looking at this pull it seems like it defeats the purpose of why this crate was created in the first place.
nb-connect was created to specifically not depend on socket2 see this issue and this pull 😕

Don't get me wrong, I understand why this pull was proposed but if we introduce socket2 as dependency to nb-connect we can replace nb-connect with socket2 in async-io directly instead of relying on nb-connect

@taiki-e
Copy link
Collaborator

taiki-e commented Feb 6, 2021

@JayceFayne Thanks for the info. I didn't know why this crate was created.

When I consider the following things, I think that it is better to depend on socket2 than maintain the partial fork of the old socket2 that contains unsafe code. (Even if socket2 contains a lot of unsafe code.)

  • socket2 is being actively developed as part of the rust-lang organization. Soundness issues like Invalid assumption of SocketAddrV{4,6} layout #1 are fixed fast enough by maintainers and the community.
  • Maintenance status of the smol project is "passively-maintained", and there is no maintainer with bandwidth to track socket2's changes.
  • socket2 is not a heavy dependency.

@smol-rs/admins What do you think?

we can replace nb-connect with socket2 in async-io directly instead of relying on nb-connect

It makes sense. After merging this PR, I would like to replace the use of nb-connect with a direct dependency on socket2.

@zeenix
Copy link
Member

zeenix commented Feb 6, 2021

``

It makes sense. After merging this PR, I would like to replace the use of nb-connect with a direct dependency on socket2.

And perhaps it'd make sense to mark nb-connect as deprecated in favour of socket2?

@taiki-e
Copy link
Collaborator

taiki-e commented Feb 7, 2021

And perhaps it'd make sense to mark nb-connect as deprecated in favour of socket2?

I thought a simple wrapper crate might be useful because we sometimes have to write a lot of cfg when using socket2, but since there are very few direct users of this crate, deprecation seems to make sense.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Invalid assumption of SocketAddrV{4,6} layout
4 participants