core/transport/memory: Return dialer address in Upgrade event #1724
Conversation
Add a unit test checking whether the dialer address reported to the listener on upgrade is unequal to the listeners own address.
Previously a `Listener` would return its own address as the `remote_addr` in the `ListenerEvent::Upgrade` event. With this commit a `Listener` returns the dialer address as the `remote_addr` in the `ListenerEvent::Upgrade` event. To do so a `DialFuture` registers a port with the global `HUB` at construction which is later on unregistered in the `Drop` implementation of the dialer's `Chan`. The sending side of the `mpsc::channel` registered in the `HUB` is dropped at `DialFuture` construction, thus one can not dial the dialer port. This mimics the TCP transport behaviour preventing both dialing and listening on the same TCP port.
There was a problem hiding this comment.
Looks good to me, I only left minor comments. It seems the current approach was intentional, supposedly mimicking the behaviour of unix domain sockets or udp sockets, though I do not understand the latter reference as UDP can certainly have different source and destination ports. I think generating ephemeral source ports is preferable.
#1721 does not mention what problems the current behaviour causes. Is it simply that the ListenerEvent::Upgrade::remote_addr is the same for all connections with the memory transport?
Co-authored-by: Roman Borschel <romanb@users.noreply.github.com>
|
I opened #1721 after looking at Substrate logs and realizing that the local address and send back address were the same. |
|
I think the behavior introduced in this pull request is at least more intuitive than the current behavior. Thus I am voting to merge this pull request. Objections? |
|
Please don't forget to update the changelog, either before merging or after. |
Thanks for the reminder @romanb! It slipped my mind. |
Previously a
Listenerwould return its own address as theremote_addrin theListenerEvent::Upgradeevent.With this commit a
Listenerreturns the dialer address as theremote_addrin theListenerEvent::Upgradeevent. To do so aDialFutureregisters a port with the globalHUBat constructionwhich is later on unregistered in the
Dropimplementation of thedialer's
Chan. The sending side of thempsc::channelregistered inthe
HUBis dropped atDialFutureconstruction, thus one can not dialthe dialer port. This mimics the TCP transport behaviour preventing both
dialing and listening on the same TCP port.
Closes #1721.