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

Replacing Grizzly with lightweight HTTP server #26

Open
wants to merge 47 commits into
base: master
Choose a base branch
from

Conversation

jtulach
Copy link
Contributor

@jtulach jtulach commented Feb 23, 2020

The Browser presenter needs an HTTP server to talk to. Original code was using Grizzly, but I find it too heavyweight. Now there is an experimental branch jtulach/WebViews and it currently contains a copy of Browser presenter - we don't want Grizzly server in the platform or IDE, right?

Given the special requirements on the browser/server communication (single-threaded JavaScript/HTTP handler is the best) - e.g. something else than regular HTTP servers are optimized for, I think it makes sense to craft own one. The code will support both servers - however, unless one provides Grizzly libraries explicitly, the simple HTTP server is going to be used:

        <dependency>
            <groupId>org.glassfish.grizzly</groupId>
            <artifactId>grizzly-http-server</artifactId>
            <version>2.3.19</version>
        </dependency>

This PR introduces simple HTTP implementation written by me. Subsequent commits need to polish and complete the implementation and make sure it can be used instead of Grizzly for the Browser presenter purposes.

Copy link

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

I have a bad feeling if people start writing their own http server because its "easy". My experience is, that somewhere something silently breaks and in the end you find, that you broke it yourself.
Given the statement:

Original code was using Grizzly, but I find it too heavyweight and fragile (when I use it in my MPD UI project, it breaks when typing fast).

Leads me to the question: What is fagile in grizzly? It is at the core of the glassfish application server and was/is used for several production sites. I'm pretty sure, that if it would be that fragile I would have seen some fallout from this.

@jtulach
Copy link
Contributor Author

jtulach commented Mar 24, 2020

There is a blog post describing how to turn the browser-presenter on.

For swing I invoke a single class, get a Frame

It is certainly simple to use Swing to develop desktop application. Is it also that simple to run swing on iOS, Android and in a webpage?

@matthiasblaesing
Copy link

I anyone else follows the blog post and fails (I got an UnsatisfiedLinkError because my Ubuntu System has no webkitget-3.0), you have to:

  • disable the javafx profile (the archetype assumes the JDK bundles FX, which was never the case for any OpenJDK (the profile is located in the file client/pom.xml)
  • force the browser to AWT, else the webkit browser is tried and blows. Add <argument>-Dcom.dukescript.presenters.browser=AWT</argument> to the configuration of the exec-maven-plugin as the first argument

@jtulach jtulach force-pushed the AnyHttpServer branch 2 times, most recently from f2ec112 to 255427e Compare March 29, 2020 17:54
@jtulach
Copy link
Contributor Author

jtulach commented Mar 29, 2020

I am satisfied with the current state of SimpleServer. It works way better for the MPDUI project than the [email protected] version. Once I get the JDK11 Travis crashes (unrelated to the server implementation) under control, I'll integrate.

@matthiasblaesing
Copy link

From me this is a -1 (as already indicated). I still don't see an analysis, that suggests, that verifies, that grizzly is the problem here. So from my perspective the underlying problem is covered, but not solved. In the long run we will end up with an unmaintained piece of code.
While trying to investigate I already ran into multiple problems that show bitrot (dependency on an outdated libwebkit, inability to run on OpenJDK 11) and in a few months noone will remember why this special http server was needed.

@jtulach
Copy link
Contributor Author

jtulach commented Mar 29, 2020

Thanks for trying to review my clean up, Matthias. If you have comments about my code changes introducing broken code, please add them next to appropriate lines.

I see it is not easy dig through all the layers that stay in a way. Feel free to report separate bugs as:

  • outdated libwebkit isn't really related to browser presenter
  • do you really have a problem with OpenJDK 11?
  • maybe just OpenJDK8? boot-fx module is known to require JDK8 with JavaFX.

In the long run we will end up with an unmaintained piece of code.

I have to say that I would rather maintain code that I have written than code that has been donated and provably doesn't work.

@matthiasblaesing
Copy link

and provably doesn't work.

You claim grizzly is broken, yet don't substantiate it ("With my code it works" is neither an analysis of the underlying problem, nor a proof).

For the record I did not do a review - a review can't be done until the problem is understood and I don't think that is the case here. I expressed, that I don't think the code is a step into the right direction.

@jtulach
Copy link
Contributor Author

jtulach commented Apr 1, 2020

You are right in both points, Matthias. First of all I haven't provided enough justification to convince you that the move away from Grizzly is good idea. Second, you haven't provided the review I was hoping for. However rather than continuing discussing soundness of my coding practices, I'd like to ask other reviewers to join. Hopefully we'll be able to focus on the code change and not how good/bad programmer I am. @eppleton, @dukescript, @jlahoda, @sdedic, @tzezula, can you please join and help me find threading bugs in SimpleServer and/or in other previously existing code?

@jtulach
Copy link
Contributor Author

jtulach commented Jul 2, 2021

I believe ceb72dd is the fix Matthias was calling for. Release 1.7.2 seems to provide stable Generic presenter even with Grizzly.

@JaroslavTulach
Copy link

Now there is an experimental branch jtulach/WebViews and it currently contains a copy of Browser presenter together with the SimpleServer code - we don't want Grizzly server in the platform or IDE, right? However, it'd be better to avoid the copy altogether (done in #42) and host the server code in HTML/Java API rather than letting other modules to sneak in via package private classes or making the pluggable server implementation a public API. In my opinion.

@matthiasblaesing
Copy link

Now there is an experimental branch jtulach/WebViews and it currently contains a copy of Browser presenter together with the SimpleServer code - we don't want Grizzly server in the platform or IDE, right?

I don't want the IDE to open listening network ports to be show a simple GUI. If we really open listening ports with the IDE to show internal UIs, we need to have a more serious discussion, because at that point the IDE becomes a security problem and raises questions like: How is access secured and what is done to ensure that the IDE can't be misused for attacks.

@JaroslavTulach
Copy link

The focus of your review shifted and I take it as a sign that replacing Grizzly is no longer a non-sense.

we need to have a more serious discussion, because at that point the IDE becomes a security problem

Alas, that's a valid concern. I'll keep that in mind before creating a PR for jtulach/WebViews. However it is not really related to the question whether such security problem would be caused by Grizzly or home made server.

@jtulach
Copy link
Contributor Author

jtulach commented Nov 29, 2021

I am facing a dilemma, Matthias. I need to integrate the jtulach/WebViews in two weeks. Launching the HTTP server is the only solution I have for providing rich refactoring UIs, but the security issue is real.

The only way I can think of to avoid it is: Create a random UID. Use it for the first connection between the browser and the server. Then stop accepting further request. That'd be safe, right? Plus stop listening on first wrong UID connection or if no connection is made in 5s...

@matthiasblaesing
Copy link

Launching the HTTP server is the only solution I have for providing rich refactoring UIs, but the security issue is real.

Ähm - NetBeans is not such a bad IDE and it is build around Swing. I know it en en vouge to bash Swing and to call it dead and all, but it is here and I don't see it dying (at least there is no valid successor visible right now). Refactoring works right now.

And yes, I see it when a HTML renderer is used, it is visually different and looks worse, than plain Swing.

The only way I can think of to avoid it is: Create a random UID. Use it for the first connection between the browser and the server. Then stop accepting further request. That'd be safe, right? Plus stop listening on first wrong UID connection or if no connection is made in 5s...

That is one way - or why not use an in-memory transport? That way you don't get zero exposure and direct interaction between the runtimes. For the OpenJFX webview a direct interaction between Java and Javascript code is possible by injecting a Java object into the JS context of the Webview. I bet other environment offer similar options, I doubt, that all webview integration go though a network port for interaction.

Copy link

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

As already written I would try not to get through the network layer to access a library (I consider a webview a library). If you can't interact with the webview directly it looks broken to me.

In general I still think, that implementing a HTTP server from scratch is not something that should be done. Maybe this is an alternative:

https://docs.oracle.com/en/java/javase/11/docs/api/jdk.httpserver/com/sun/net/httpserver/HttpServer.html

Its a minimal http server embedded in the JDK. Reading the @SInCE info it is there since SOAP support was implemented (at least I think I remember such a comment) and given JEP 408 and that the class was extended in 18 it is unlikely to go away anytime soon.

context = Collections.emptyMap();
}

Matcher length = PATTERN_LENGTH.matcher(header);

Choose a reason for hiding this comment

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

Is it ensured, that there can no be chunked transfer happening?

Choose a reason for hiding this comment

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

Yes, there is

  int fullHeader = buffer.indexOf("\r\n\r\n");
  if (fullHeader == -1) {
    return null;
  }

e.g. no processing happens until the end of header is received.

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