Skip to content

Added Unix Domain Socket support.#991

Closed
0xIO32 wants to merge 1 commit into
PaperMC:dev/3.0.0from
0xIO32:dev/3.0.0
Closed

Added Unix Domain Socket support.#991
0xIO32 wants to merge 1 commit into
PaperMC:dev/3.0.0from
0xIO32:dev/3.0.0

Conversation

@0xIO32

@0xIO32 0xIO32 commented Apr 2, 2023

Copy link
Copy Markdown

It adds Unix Domain Socket support, so that it don't require a tcp port to be binded to work.

@astei

astei commented Apr 2, 2023

Copy link
Copy Markdown
Contributor

This is a pretty clear API breakage (not to mention the fact it requires us to expose internal implementation details to plugins), and while it's been planned for the next major revision of Velocity, it's not been a high priority for a few reasons (mainly due to lack of time on my part).

@0xIO32

0xIO32 commented Apr 2, 2023

Copy link
Copy Markdown
Author

I think it shouldn't break the api. In the only class in the api that i modified, i only added methods or modified private fields and added constructors, but removed or modified no existing components (except for the "ServerInfo" constructor to use the SocketAddress type as parameter and not the InetSocketAddress type, which shouldn't break something, because the type InetSocketAddress extends from it)

@astei

astei commented Apr 2, 2023

Copy link
Copy Markdown
Contributor

Modifying the existing 2-ary constructor of ServerInfo constitutes API breakage. The class specifically asks for InetSocketAddress. While this is source code compatible (I can compile against this change), it's not binary compatible.

In addition, getAddress() is now broken - it's now possible that a ClassCastException can be thrown by the JVM, since you cannot cast DomainSocketAddress to an InetSocketAddress.

@0xIO32

0xIO32 commented Apr 2, 2023

Copy link
Copy Markdown
Author

Yes i could add a third constructor with this type and for the getAddress() method, i could add a change, that the InetAddress.getLoopbackAddress() ( "127.0.0.1" ), will be used when it is a DomainSocketAddress. That would than break nothing?

@0xIO32 0xIO32 force-pushed the dev/3.0.0 branch 2 times, most recently from d92c244 to 025f779 Compare April 2, 2023 22:12

@hugmanrique hugmanrique left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't see how to avoid the getAddress breakage sensibly. We will have to wait a bit, iirc I've seen some talks about a near-future major release launch in Discord.

I would prefer to see the commit mentioned in the other comment backported, since astei already tested those changes (the address parsing portion of this PR is a perfect candidate for some unit tests).

Comment thread api/src/main/java/com/velocitypowered/api/proxy/server/ServerInfo.java Outdated
Comment thread api/src/main/java/com/velocitypowered/api/proxy/server/AddressType.java Outdated
@0xIO32

0xIO32 commented Apr 3, 2023

Copy link
Copy Markdown
Author

Now i removed the getAddress method, the AddressType enum and the constructor which was only used for not breaking the api.

@0xIO32 0xIO32 requested a review from hugmanrique April 3, 2023 13:18
astei added a commit that referenced this pull request Apr 4, 2023
Closes #991. We can't properly support this without an API break, so let's just do it in 5.0.0.
@0xIO32 0xIO32 closed this Apr 4, 2023
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.

3 participants