-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Regression] 12.0.7 breaks junixsocket-jetty due to Selector logic change #11500
Comments
@kohlschuetter the problem is caused by the fact that before 12.0.7, the
You have overridden In Jetty 12.0.7, the
The
You can Or, you can embrace the new model and create your own |
Out of curiosity, did you try to implement a custom In this way, a call to I'd imagine that by using a custom |
Thanks for your quick reply, @sbordet!
Yes, junixsocket has its own I have a working workaround now, indeed using a custom I'm working on integrating the fix into junixsocket as we speak. The problem I see here, however, is that the above commit in question introduced an unexpected breaking change in a patch-level release (12.0.x). I wonder if such changes should automatically trigger a minor version bump (12.0.6 -> 12.1.0)? If the breakage was undesired, can we find a fix that is backwards compatible? |
Jetty does not directly use the default The problem is that you integrated with a very low level part of Jetty that is somehow internal, but it cannot really be for technical reasons (Java visibility etc.). Your use case is pretty unique. Unfortunately, to fix the Unix-Domain issue we had to change these internals, and you have been affected. We believe all other use cases where a My point being that the exception is thrown by your code, not Jetty's, because it receives an unexpected Perhaps that test runs without the custom For example, you should not need to do this: This also looks suspicious? |
The two lines you referred two are indeed how I made jetty compatible with junixsocket. Now with 12.0.7 only one of them is called (the first one, responsible for the The culprit is here ( Lines 216 to 225 in 369d9f7
in combination with jetty.project/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnector.java Lines 642 to 650 in 369d9f7
|
I do not understand. The snippets you refer to are just creating a The If you have correctly replaced the |
jetty 12.0.7 introduced some backwards-incompatible changes that broke AFSocketClientConnector. Add support for a junixsocket-specific Transport implementation, and modify the jetty support code in a way that is both compatible with jetty 12.0.7 as well as backwards-compatible as before. jetty/jetty.project#11500
See my changes to junixsocket-jetty above how I restored compatibility with all Jetty versions. It was definitely not straightforward but it works now. The problem with the It would be great if I could use my custom |
@kohlschuetter I don't understand. Did you replace the default If you have not replaced the default |
No, junixsocket does not replace the default junixsocket provides its own implementation that only works with its own socket implementations, that support protocol families such as AF_UNIX, AF_TIPC, AF_VSOCK, etc. See The main reasons we don't override the default The standard Java API makes no guarantees that there is only a singleton |
Ok, so you don't replace the Then, how can Jetty know how to create your own Either you have to give Jetty a custom IIUC, before it was working because you had a custom Note that the custom My understanding is that you must have a custom Do you have cases where you use your custom |
Yes, with 12.0.7, I currently need both a custom With the new That would allow me to share the same client for both TCP-related requests and junixsocket-related ones.
Only for jetty < 12.0.7 compatibility
Having just one SocketAddress is not a big deal for my use case. I'm serving HTTP over (for example) AF_UNIX, AF_TIPC and AF_VSOCK, at the protocol level replacing TCP/IP, but still using regular hostnames to identify the target. We can resolve the target host at the HTTP level ( Currently, thanks to |
Just to be clear: you need a custom
See above, we don't want to do that. We want to be able to specify a
Not sure I follow.
This is a good point. I may add Can you please open a new issue about this? |
I see a problem with that because (as seen) you cannot guarantee that any Perhaps you could change |
Jetty version(s)
12.0.7
Jetty Environment
any
Java version/vendor
any, e.g. openjdk version "21"
OS type/version
any, e.g. macOS
Description
commit 24c1140 ("Fixes #8979 - Jetty 12 - HttpClientTransport network "modes". (#11368)") introduced a breaking change compared to 12.0.6.
junixsocket-jetty-12 now has failing tests (a junixsocket Selector tries to register a key on a vanilla Java / non-junixsocket SocketChannel, which is an unexpected situation).
In issue #8979, @sbordet writes:
I guess that assumption does not hold true for custom Socket implementations like junixsocket's ...
How to reproduce?
check out
https://github.com/kohlschutter/junixsocket/tree/main/junixsocket-jetty-12
modify its POM (change <jetty.version> to 12.0.7 or 12.0.7-SNAPSHOT)
mvn clean install
The text was updated successfully, but these errors were encountered: