Huge Update #2

[LeonMatthes]

Implement the changes requested by @futile on #6 .

Additionally, the PeerID is now a separate type, so people can't just pass in arbitrary numbers into the peer indexing.

I chose for now to keep Index and IndexMut in Host. As now the primary way to access a peer is through the host via its PeerID, indexing is just a lot more convenient (see examples/client.rs).\ If you don't like this, I can of course remove the impl's again.

I decided not to touch the constructors of Peer, it does feel quite hacky, but also cleans up the code a fair bit...

[LeonMatthes]

Using a boxed slice instead of Vec could have negative performance implications in some cases. If you're working with a serialization library such as bincode it's likely that you will get a Vec<u8> as the result of serialization and not Box<[u8]>, so you need to "convert" it to a boxed slice which involves dropping the Vecs capacity.

My expectation on this was that into_boxed_slice() would likely return in O(1), as it technically doesn't have to move the memory around to shrink it. It just needs to tell the allocator to reduce the amount of reserved memory. However, you're right. The current implementation in libcore::alloc is unable to do so and copies the data with O(n), negating any potential improvements by not copying the data into enet, whenever capacity != size. I will revert those changes back to Vec. Vec<> is much more common than Box<[]> anyways, as you noted as well.

@htrefil @futile, note though that the implementation in #6 still contains a memory error. You're dropping the Vec<>, instead of forgetting it, which leads to deallocation of the memory by Rust, leaving enet to SegFault on the already freed data. That needs to be fixed should you prefer to merge #6 instead of #9 .

[bowbahdoe]

Updates on this?

[LeonMatthes]

Updates on this?

You might want to take a look at the moduleverse branch of my fork. https://github.com/LeonMatthes/enet-rs/tree/moduleverse

It contains both the fix from this PR + the endianness fix in #10 . Plus Some additional conveniences like derive(Hash) on Address and some Send and Sync-impls. I've used it quite successfully in a project for the last months.

[SeanTolstoyevski]

what is last status?

[949f45ac]

You might want to take a look at the moduleverse branch of my fork.

Hey @LeonMatthes , thanks for your work on this! While the repo maintainer is unresponsive, would you consider publishing your fork on crates.io under a different name, so that it can more conveniently be included in projects? Kind regards

[LeonMatthes]

Hey @LeonMatthes , thanks for your work on this! While the repo maintainer is unresponsive, would you consider publishing your fork on crates.io under a different name, so that it can more conveniently be included in projects? Kind regards

I'd prefer to not pollute the crates.io namespace if possible. For now, you can just add this to your Cargo.toml:

enet={git="https://github.com/LeonMatthes/enet-rs.git", branch="moduleverse"}

Which should be easy enough :blush:

@futile If you don't want to continue supporting this repository, you could give me maintainer access to this crate on crates.io and I could continue work on my fork.

[JohnDowson]

While we're at it, Connect is supposed to have data just like Disconnect does.

[LeonMatthes]

@futile Great to see you're back :)

Unfortunately this PR has now deviated too far from your work for a rebase to be feasible. (I'm trying right now, but it's just way too cumbersome). I can merge your master branch into this PR though if you're still interested in these changes.

I know it's a large PR, but I think this really "rounds things off" nicely. If you want we could also do a zoom call or similar, so we could go through the changes quickly.

This fork is still working great in my own project, over 1 year of use now. The best thing I have to say about it is that I didn't have to touch the code again since creating this PR. Which probably also means the code is likely to be easy to maintain.

[LeonMatthes]

While we're at it, Connect is supposed to have data just like Disconnect does.

Probably best to keep stuff like that in a separate PR, so this PR doesn't balloon any further.

[JohnDowson]

Probably. I guess I'll wait until this gets merged and then push my patches.

[futile]

Hey everyone, and sorry for the long silence! :) And thanks a lot for your contributions and discussions! I have had time to come back to this since the last few weeks, and plan to see this PR through as well; I haven't looked too closely at the changes again yet, but I'll have time to do so over the next few days. @LeonMatthes thanks for the offer! I'll get back to you once I've taken a look at all the changes. The fact that you've been using the library with your changes for quite some time without needing much else is really useful feedback though! :)

[futile]

Okay, just took a look at the changes again, and while I think that there are still some things to discuss/change, I think the changes are useful in general. @LeonMatthes can you merge master into this PR/your moduleverse branch and either push to this PR or open up a new one? I think that would be a good basis for discussion. Thanks!

Some stuff I think still needs some adjustments:

• PeerIDs are useful, but I'm not sure if ENet reuses IDs/slots in the peers-array. If it does, I would like some kind of generation to be added to PeerIDs, so old/stale PeerIDs don't accidentally still "work", but refer to other Peers. If ENet doesn't reuse IDs/slots then this won't be needed, but I think its sensible to check this, because otherwise we basically end up with dangling pointers again (just in the form of IDs).
• I think the Index and IndexMut impls for Host should go, as a Host isn't really a "list"/container of peers. If we want something like this, we could make a type Peers that can be retrieved from a Host, which can be indexed, iterated over, etc.. But I don't think this is necessary for now, Host::peer() and Host::peer_mut() seem ergonomic enough to me; But open for discussion here :)
• Probably some cleanup of the history will also be necessary, the PR is already at 34 commits for ~ +350/-150 changes, which is too much imo. BUT since the branches have already diverged too far for a rebase anyway, we can just worry about this after everything else has been discussed. One solution I can see would be to simply take the whole diff once everything else has been resolved, and turn that into individual (new) commits, which then have clear scopes, commit messages, etc..

Once again, thanks for the PRs/discussion, much appreciated!

[LeonMatthes]

Having generations for peers is definitely useful, I was concerned about "dangling" PeerIDs as well. AFAIK enet reuses them. I'll have to figure out a way to count the generations though. I'll see when I have time to address your change requests. I'll also remove the Index/IndexMut.

[LeonMatthes]

And thank you for reviewing this, it's really gotten too big over time. I completely understand that it's a big ask of any maintainer to work with so much code that's just dumped on your head.

Thank you!

[JohnDowson]

IMHO, worrying about generational indexing could be postponed until another PR.

[futile]

@LeonMatthes thanks a lot! Feel free to just merge master into your branch and push that to this/a new PR to get started! :)

IMHO, worrying about generational indexing could be postponed until another PR.

The thing is, switching to non-generational indices would make the library interface less correct than it is now, which is something I generally avoid. I also don't think generational indices are that much of an (implementational) overhead, a simple version would just keep a Vec or a HashMap (or sth. similar) around to keep track of the current index, adjust it for connects/disconnects, and check it when indexing. If there are some issues I'm not seeing here, I would rather move PeerIDs to an individual PR, so the rest of this can be merged independently. However, I'd think that that's not necessary, and it should be fine to do that in this PR, since it already includes most of the index infrastructure.

But I agree that other new things should probably not be added to this PR anymore, so the scope stays where it is now.

Also, if you have some other changes you would like to get merged, feel free to put up a PR, and we can see whether the PRs overlap/in which order we should merge. If your changes are already based on @LeonMatthes's code, then it's probably best to wait for this PR I guess 😅 :)

[LeonMatthes]

Hm, I've made the PeerID generational. I've moved the drop_disconnected call from process_event to service, as otherwise the stale Peer would not be dropped until a new Event is received. Having the Peer be invalidated at the next service is much easier to understand in my opinion.

There is however, a problem: disconnect_now currently doesn't increment the Peer generation. The peer doesn't have any way to access the Host, so it can't do the increment itself.

I'm considering storing the generation in the Peer data itself, that probably has its own problems... Will have to give this some more thought. Any suggestions appreciated as well.

[LeonMatthes]

@futile Sorry that it has taken me so long to get back to this. I've incorporated your feedback wherever I didn't have questions and marked the conversations as resolved. There's basically just a small question remaining about whether to continue using Duration in the API.

Considering that i.e. forgetting a std::Vec and other data structures in Rust should always be considered carefully, I think using a Drop impl for cleanup here is fine. I've added a comment nonetheless, like you suggested.

[Jachdich]

Is there any update on this? Thinking of using this library in my project and I'm just curious as to the sate of development.

[ryanmcgrath]

Seconding @Jachdich - curious if this is close to being merged?

[michidk]

Wow, this PR is great - exactly what I was looking for. Looking at this PR, I would rather use this instead of the current master. Sometimes it's unavoidable to publish a forked crate. I would rather have multiple forks on crates.io than one outdated one. @LeonMatthes what exactly is the difference between your moduleverse branch and this PR (your master branch)? moduleverse contains some older changes, right? @futile is there anything preventing this from being merged and uploaded to crates.io?

[LeonMatthes]

@LeonMatthes what exactly is the difference between your moduleverse branch and this PR (your master branch)? moduleverse contains some older changes, right? @futile is there anything preventing this from being merged and uploaded to crates.io?

Yeah, the moduleverse branch is a bit outdated now, it doesn't contain the generational PeerID yet. It does however contain some Send, Sync and Hash implementations that aren't in this master branch yet. Should probably open a separate PR for them... If you don't need the Send,Sync or Hash impls that I added to the moduleverse branch, working with the branch of this PR should work well. Can't guarantee any further breaking changes though.

I've been battling a bindgen-issue with enet-sys after updating my Fedora yesterday. see: https://github.com/ruabmbua/enet-sys/issues/19

Once the enet-sys stuff is sorted out, I'll consider uploading my own version to crates.io (maybe enet-rs-next?). Want to avoid that if at all possible though, as it's much better for the community to not have to deal with multiple forks for the same thing. For now, feel free to just use:

enet-rs={git="https://github.com/LeonMatthes/enet-rs.git"}