Skip to content

Makes all objects in connection have a stable address #285

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

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

anarthal
Copy link
Collaborator

No description provided.

@anarthal
Copy link
Collaborator Author

This is intended to alleviate one of the problems you had in #283, and make reasoning about lifetimes trivial.

Before I add some tests to verify that moving connections is fine, do you think this is the way to go?

I could also make exec_op, reader_op, writer_op and friends operate on connection_impl rather than connection, which should be marginally more efficient, but requires more work. Let me know if you think it's worth it.

@mzimbres
Copy link
Collaborator

AFAICS this is only needed if we want to support a connection being moved while async_run is pending. Or is there anything else? Is it reasonable to allow that?

@anarthal
Copy link
Collaborator Author

It's aimed to solve things like this: #283 (comment)

This would cause a segfault after moving the connection, whether you perform the move before, during or after async_run executes. IMO not having to reason about lifetimes at all makes things much easier.

@mzimbres
Copy link
Collaborator

Hi Ruben, I am going to address those comments in the way you suggested. Just don't have much time now. I agree what you propose here is good (and common) practice and does not have any bad side effects. I support further development here, thanks.

@anarthal
Copy link
Collaborator Author

Great. I will do some further fixes here and post it for review again.

I'm going to be out of office during the weekend too, so don't worry.

@anarthal anarthal marked this pull request as draft July 16, 2025 11:31
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