-
Notifications
You must be signed in to change notification settings - Fork 178
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
refactor(iroh-relay): simplify structure in clients #3100
Conversation
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3100/docs/iroh/ Last updated: 2025-01-07T10:47:41Z |
drop(client); // avoid deadlock | ||
self.unregister(dst).await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps self.unregister
should take the client: Ref<...>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to drop it in there? that seems uneccessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this suggestion, that could make it less dangerous?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed that having unregister
take the client we are doing something programatically to prevent this from happening again during a future refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a few more minutes...
drop(client); // avoid deadlock | ||
self.unregister(dst).await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this suggestion, that could make it less dangerous?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion is a nit tbh. I still think it's good, but I also think getting this merged is a pretty high priority and I don't want this to be blocked on that nit.
Description
Breaking Changes
Notes & open questions
Change checklist