ARQ-2194 Tomcat 9 embedded starts without connector by default, we ne…

[manovotn]

…ister one ourselves.

JIRA link - https://issues.jboss.org/browse/ARQ-2194

I was trying to utilize tomcat-embedded-8 adapter with Tomcat 9 (in Weld servlet testing)
Turns out there is one change needed during startup, Tomcat 9 by default does not register any connectors (e.g. listens on no ports). Adding this line fixes it for Tomcat 9 and it still works with Tomcat 8.

Happy to provide any additional details/info/changes.

[manovotn]

@MatousJobanek @bartoszmajsak Can some of you guys look into this, please?

[bartoszmajsak]

Thanks for the PR @manovotn. I'm just wondering how calling tomcat.getConnector(); fixes that? Is this getter containing some logic which registers connectors if none defined?

Also - should we think about having Tomcat 9 test suite in here?

[bartoszmajsak]

As for my first question - I think this thread explains what is going on under the hood https://bz.apache.org/bugzilla/show_bug.cgi?id=60368#c1

[manovotn]

Yea, the getter registers the default connector which is exactly the same as what Tomcat 8 does automatically if you register none. That's why this change works for both.

[manovotn]

What kind of testsuite would you imagine?

I didn't want to create a new tomcat9 directory as there are no huge changes and I would just be copy-pasting the same code.
Also, to properly test Tomcat 9, we should maybe test with Servlet 4.0 protocol but Arq. doesn't support that yet but 3.0 works just fine anyway.

We could maybe add a maven profile with tomcat 9 and run pretty much the same tests that are there for 8?

[bartoszmajsak]

We could maybe add a maven profile with tomcat 9 and run pretty much the same tests that are there for 8?

That is something I had in mind as a good start. This way we could have been able to detect this problem before it hurt you :) Could you add it to this PR?

[manovotn]

Sure, will look into it

[manovotn]

@bartoszmajsak So I remade that pom.xml - now it has default tomcat8 profile which works just like it did before and an additional tomcat9 profile which you can manually choose to run via -Dtomcat9.

Note that Tomcat 9 no longer needs extra logging artifact(tomcat-embed-logging-juli), so it is isolated in tomcat8 profile only.
And I also had to bump version of animal-sniffer plugin to make it stop failing with Tomcat 9.

Any ideas on how to convince surefire to execute the TS on both tomcats?

[manovotn]

Or we could just add a line to Travis to execute these tests again with a profile...

[bartoszmajsak]

Any ideas on how to convince surefire to execute the TS on both tomcats?

@manovotn one way would be defining multiple executions as part of the build/surefire step:

https://gist.github.com/bartoszmajsak/6cacaf40f0abd7d70e4c28165bd1ab24#file-pom-xml-L51-L86

[manovotn]

Hmm, tomcat-embed-logging-juli doesn't exist for 9+ (was merged to another artifact I think)...but that could be excluded perhaps. Will give it a spin tomorrow.

[bartoszmajsak]

I'm adding version for 8... in provided scope. A bit ugly, but saves us from profiles madness.

[manovotn]

@bartoszmajsak that doesn't work. It is failsafe plugin doing the execution (not surefire) but mainly, the property does not change in the execution hence it just runs twice with Tomcat 8.

Here is what I tried - https://gist.github.com/manovotn/497707377dbda83671574b5671f355ec

[manovotn]

@bartoszmajsak alternatively, we could create a tomcat-embedded-9 sub project which just takes the sources and tests from 8 and executes them on 9.
Something like this - https://github.com/manovotn/arquillian-container-tomcat/tree/tomcat9embFix_v2
It is not the nicest of solutions, but works. WDYT?

[bartoszmajsak]

Make sense 👍. It would also avoid the confusion of using Tomcat 8 container for Tomcat 9. I think it's fine. Go ahead with this change on this PR. Many thanks!

[manovotn]

Done

Not sure why GH claims this branch isn't updated, I have just rebased it.

[bartoszmajsak]

@manovotn there are other small PRs coming in with dep updates done by @dependabot[bot], most likely that's the reason.

[bartoszmajsak]

LGTM! Thanks for your contribution.

[manovotn]

@bartoszmajsak no problem, do you think you would be able to cut a release any time soon?

[bartoszmajsak]

New version 1.1.0.Final is out and available in central. Thanks for your contribution again!