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

7494: Unable to view JMC Help Contents #347

Closed
wants to merge 1 commit into from

Conversation

cimi
Copy link
Contributor

@cimi cimi commented Jan 9, 2022

This PR fixes a problem that makes all JMC help pages fail.

We use the eclipse help plugin, it plugin starts a web server which renders the XML content in the docs plugin via JSP. When the user clicks help in the JMC UI they are sent to the browser, which tries to render one of these JSP pages.

In #306 we introduced new dependencies on multiple Jetty packages so we can run a websocket server inside JMC. After these changes we notice that the help pages fail to render with the following error:

javax.servlet.ServletException: non-HTTP request or response
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:578)
	at org.eclipse.equinox.http.jetty.internal.HttpServerManager$InternalHttpServiceServlet.service(HttpServerManager.java:308)
	at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:764)
	at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:508)
	at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:221)

This is the code that throws the exception:

    @Override
    public void service(ServletRequest req, ServletResponse res) throws ServletException, IOException {
        HttpServletRequest request;
        HttpServletResponse response;

        if (!(req instanceof HttpServletRequest && res instanceof HttpServletResponse)) {
            throw new ServletException("non-HTTP request or response");
        }

        request = (HttpServletRequest) req;
        response = (HttpServletResponse) res;

        service(request, response);
    }

The help plugin has Jetty as a transitive dependency so it's likely this error is caused by the new servlet-api dependency that we've introduced. Using the debugger, we can set a breakpoint before the exception is thrown and we can inspect req and res to see why the instanceof check fails. The result of this is very strange:

Screenshot 2022-01-08 at 18 05 43

I don't understand why this fails here, since we can see with reflection that req implements the expected javax.servlet.http.HttpServletRequest interface. It's likely this is because we have two servlet-api bundles loaded, org.eclipse.jetty.servlet-api (explicit dependency of flightrecorder.ui) and jakarta.servlet-api (transitive dependency of the help plugin).

I also don't understand if it's possible that instanceof fails because it tries to compare javax.servlet.http.HttpServletRequest loaded from the different bundles, given that the package and interface names are identical.

Regardless of the root cause, removing the explicit dependency on org.eclipse.jetty.servlet-api from the target platform and depending on jakarta.servlet-api in the flightrecorder.ui manifest fixes the issue. Now help pages load successfully and the websocket server works as before.


Progress

  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JMC-7494: Unable to view JMC Help Contents

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jmc pull/347/head:pull/347
$ git checkout pull/347

Update a local copy of the PR:
$ git checkout pull/347
$ git pull https://git.openjdk.java.net/jmc pull/347/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 347

View PR using the GUI difftool:
$ git pr show -t 347

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jmc/pull/347.diff

We needed a servlet api implementation for the websocket server
so we added a dependency on the jetty.servlet-api. We already
have jakarta.servlet-api in our dependencies, required by the
eclipse help page which also uses jetty to serve dynamic content.

We need to avoid loading multiple servlet-apis because it causes
errors serving help pages - the server fails to recognise HTTP
requests, presumably because of the multiple implementations available.
@bridgekeeper
Copy link

bridgekeeper bot commented Jan 9, 2022

👋 Welcome back cimi! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Jan 9, 2022
@mlbridge
Copy link

mlbridge bot commented Jan 9, 2022

Webrevs

@openjdk
Copy link

openjdk bot commented Jan 9, 2022

@cimi This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

7494: Unable to view JMC Help Contents

Reviewed-by: hirt, aptmac

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@thegreystone, @aptmac) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Jan 9, 2022
@cimi
Copy link
Contributor Author

cimi commented Jan 10, 2022

/integrate

@openjdk openjdk bot added the sponsor label Jan 10, 2022
@openjdk
Copy link

openjdk bot commented Jan 10, 2022

@cimi
Your change (at version fe4d63f) is now ready to be sponsored by a Committer.

@cimi
Copy link
Contributor Author

cimi commented Jan 10, 2022

Since the ticket mentioned the help pages didn't work if we run JMC with JDK 17, I tried it out. I could reproduce it, here is the error:

org.apache.jasper.JasperException: PWC6033: Error in Javac compilation for JSP

PWC6197: An error occurred at line: 41 in the jsp file: /index.jsp
PWC6199: Generated servlet error:
System cannot be resolved


	at org.apache.jasper.compiler.DefaultErrorHandler.javacError(DefaultErrorHandler.java:129)
	at org.apache.jasper.compiler.ErrorDispatcher.javacError(ErrorDispatcher.java:299)
	at org.apache.jasper.compiler.Compiler.generateClass(Compiler.java:392)
	at org.apache.jasper.compiler.Compiler.compile(Compiler.java:453)
	at org.apache.jasper.JspCompilationContext.compile(JspCompilationContext.java:625)
	at org.apache.jasper.servlet.JspServletWrapper.service(JspServletWrapper.java:374)

I didn't know how to open the jsp in eclipse so I couldn't play with it there, but looking at the source it seems that because of the System.getProperty call the JSP fails to compile. Searching online for similar problems, I found this issue which suggests upgrading JDT (and specifically the eclipse compiler) might fix it.

aptmac
aptmac approved these changes Jan 10, 2022
@aptmac
Copy link
Member

aptmac commented Jan 10, 2022

I didn't know how to open the jsp in eclipse so I couldn't play with it there, but looking at the source it seems that because of the System.getProperty call the JSP fails to compile. Searching online for similar problems, I found this issue which suggests upgrading JDT (and specifically the eclipse compiler) might fix it.

Guru had opened an Eclipse bug for JDT explaining our issue [0], there hasn't been a lot of comments on it recently though. Interesting that the update to JDT 3.26.0 fixed the issue for open-library, we're currently consuming 3.18 via 2021-06 [1] (and it looks like even through to 2021-12 the same version will be used).

[0] https://bugs.eclipse.org/bugs/show_bug.cgi?id=574937
[1] https://download.eclipse.org/releases/2021-06/202106161001/features/

@thegreystone
Copy link
Member

thegreystone commented Jan 10, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Jan 10, 2022

Going to push as commit b65e351.

@openjdk
Copy link

openjdk bot commented Jan 10, 2022

@thegreystone @cimi Pushed as commit b65e351.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@cimi cimi deleted the cimi/JMC-7494-fix-help-page branch Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants