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

Rename "announced ip" to "announced address" #1324

Merged
merged 13 commits into from
Feb 5, 2024

Conversation

ibc
Copy link
Member

@ibc ibc commented Feb 1, 2024

Fixes #1323

  • Rust: In TransportListenInfo rename announced_ip to announced_address.
  • Rust: In IceCandidate rename ip to address.
  • Rust: In TransportTuple rename local_ip to local_address.
  • Node: In TransportListenInfo rename announcedIp to announcedAddress and keep backwards compatibility.
  • Node: In IceCandidate rename ip to address and keep backwards compatibility.
  • Node: In TransportTuple rename localIp to localAddress and keep backwards compatibility.

NOTE: Why has been TransportTuple.localIp renamed to TransportTuple.localAddress? Because indeed its value is an announced address if a hostname was given in announcedAddress in a TransportListenInfo.

Fixes #1323

- Rust: In `TransportListenInfo` rename `announced_ip` to `announced_address`.
- Rust: In `IceCandidate` rename `ip` to `address`.
- Rust: In `TransportTuple` rename `local_ip` to `local_address`.
- Node: In `TransportListenInfo` rename `announcedIp` to `announcedAddress` and keep backwards compatibility.
- Node: In `IceCandidate` rename `ip` to `address` and keep backwards compatibility.
- Node: In `TransportTuple` rename `localIp` to `localAddress` and keep backwards compatibility.

**NOTE:** Why has been `TransportTuple.localIp` renamed to `TransportTuple.localAddress`? Because indeed its value is an announced address if a hostname was given in `announcedAddress` in a `TransportListenInfo`.
@ibc
Copy link
Member Author

ibc commented Feb 1, 2024

TODO: Once merged/released, docs in website must be updated.

@ibc ibc requested review from jmillan and nazar-pc February 1, 2024 18:30
@ibc
Copy link
Member Author

ibc commented Feb 1, 2024

Missing thing, local_address in TransportTuple in Rust must not be an IPAddr but a String. On it.

@ibc ibc marked this pull request as draft February 1, 2024 18:39
rust/src/data_structures.rs Outdated Show resolved Hide resolved
@ibc
Copy link
Member Author

ibc commented Feb 2, 2024

I need help to resolve this:

error[E0507]: cannot move out of dereference of `parking_lot::lock_api::MutexGuard<'_, parking_lot::RawMutex, TransportTuple>`
   --> rust/src/router/pipe_transport.rs:727:9
    |
727 |         *self.inner.data.tuple.lock()
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ move occurs because value has type `TransportTuple`, which does not implement the `Copy` trait

error[E0382]: borrow of moved value: `tuple`
   --> rust/src/router/plain_transport.rs:691:56
    |
688 |                         Notification::Tuple { tuple } => {
    |                                               ----- move occurs because `tuple` has type `TransportTuple`, which does not implement the `Copy` trait
689 |                             *data.tuple.lock() = tuple;
    |                                                  ----- value moved here
690 |
691 |                             handlers.tuple.call_simple(&tuple);
    |                                                        ^^^^^^ value borrowed here after move

In the first error I've tried *self.inner.data.tuple.clone().lock() but:

error[E0599]: no method named `clone` found for struct `parking_lot::lock_api::Mutex` in the current scope
   --> rust/src/router/pipe_transport.rs:727:32
    |
727 |         *self.inner.data.tuple.clone().lock()
    |                                ^^^^^ method not found in `Mutex<RawMutex, TransportTuple>`

This mutex/lock thing is above my (increasing) little knowledge, so @nazar-pc help plz.

@nazar-pc
Copy link
Collaborator

nazar-pc commented Feb 2, 2024

I think you want to clone inner data, in which case it should be lock().clone()

@ibc
Copy link
Member Author

ibc commented Feb 2, 2024

I think you want to clone inner data, in which case it should be lock().clone()

I tried it already but...

error[E0614]: type `TransportTuple` cannot be dereferenced
   --> rust/src/router/pipe_transport.rs:727:9
    |
727 |         *self.inner.data.tuple.lock().clone()
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@nazar-pc
Copy link
Collaborator

nazar-pc commented Feb 2, 2024

.lock() method returns something like MutexGruard. This is a wrapper type that locks unlocks Mutex when dropped. This MutexGuard can be dereferenced to the type mutex contains. So *mutex.lock() is essentially similar to this:

let number = 5;
let number_copy = *&number;

Since the type you have is no longer Copy, you need to clone it explicitly.

So what was *self.inner.data.tuple.lock() before (can be also written as self.inner.data.tuple.lock().clone() explicitly) now needs to be written as self.inner.data.tuple.lock().clone() due to explicit cloning.

So essentially you'll do this:

let s = "Hello".to_string();
let s_copy = (&s).clone();

@ibc
Copy link
Member Author

ibc commented Feb 2, 2024

So indeed replacing *self.inner.data.tuple.lock() with self.inner.data.tuple.lock().clone() works although I cannot understand how both things can return same type. AFAIU xxxx and xxxx.clone() return the same type, so how can *xxxx and xxxx.clone() return the same type as well?

@ibc
Copy link
Member Author

ibc commented Feb 2, 2024

In addition I've also fixed another issue as follows:

error[E0382]: borrow of moved value: `tuple`
   --> rust/src/router/plain_transport.rs:691:56
    |
688 |                         Notification::Tuple { tuple } => {
    |                                               ----- move occurs because `tuple` has type `TransportTuple`, which does not implement the `Copy` trait
689 |                             *data.tuple.lock() = tuple;
    |                                                  ----- value moved here
690 |
691 |                             handlers.tuple.call_simple(&tuple);
    |                                                        ^^^^^^ value borrowed here after move

Solution in line 689:

*data.tuple.lock() = tuple.clone();

Hope it's ok. IMHO it is since it follows same rationale as discussed in above comments.

@nazar-pc
Copy link
Collaborator

nazar-pc commented Feb 2, 2024

MutexGuard doesn't implement Clone, so compiler will try to call Clone on the type that it dereferences to, which is inner value you actually want.
https://doc.rust-lang.org/book/ch15-02-deref.html

@ibc ibc marked this pull request as ready for review February 2, 2024 11:21
@ibc
Copy link
Member Author

ibc commented Feb 2, 2024

Ok this is ready for review. @nazar-pc please.

Copy link
Collaborator

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may not be looking carefully, but looks like TypeScript doesn't process announcedIp correctly now. It seems to send it to worker, but worker only cares about announcedAddress now. You should add something like announcedAddress = announcedAddress || announcedIp on TypeScript side and not send announcedIp to worker anymore.

Comment on lines 88 to 91
announced_address: self
.announced_address
.clone()
.map(|address| address.to_string()),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two allocations look strange. I'm pretty sure this will also work:

Suggested change
announced_address: self
.announced_address
.clone()
.map(|address| address.to_string()),
announced_address: self
.announced_address
.as_ref()
.map(|address| address.to_string()),

Though this is not new, it was like this before, I just comment on what I noticed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean that this thing should also be done in other changes I did?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, but I was commenting just about this one

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I don't see any other place in which as_ref() can be used instead of cloning.

@ibc
Copy link
Member Author

ibc commented Feb 2, 2024

Moving back to draft PR because I forgot lot of things.

@ibc ibc marked this pull request as draft February 2, 2024 12:17
@ibc
Copy link
Member Author

ibc commented Feb 5, 2024

@nazar-pc, I should also update Changelog and versions in Rust, right?

@nazar-pc
Copy link
Collaborator

nazar-pc commented Feb 5, 2024

@nazar-pc, I should also update Changelog and versions in Rust, right?

Yes, please

@ibc ibc marked this pull request as ready for review February 5, 2024 08:45
@ibc
Copy link
Member Author

ibc commented Feb 5, 2024

Rust versions bump done.

worker/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me

@ibc ibc merged commit 20a606a into v3 Feb 5, 2024
20 checks passed
@ibc ibc deleted the rename-announced-ip-to-announced-address branch February 5, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Rename "announced ip" to "announced address"
2 participants