HttpServer#createContext() catches Error

[DaveCTurner]

Today we use com.sun.net.httpserver.HttpServer for writing test fixtures that emulate various third-party HTTP services. Unfortunately the handlers are executed in a context that catches and swallows Throwable, so that server-side assertions do not directly cause tests to fail.

I would like to forbid the bare use of HttpServer#createContext and instead provide a utility method that does the same thing except that it adds a wrapper which uses ExceptionsHelper.maybeDieOnAnotherThread to fail tests on an Error. The trouble is that the whole com.sun.net.httpserver.* is already forbidden by forbidden-APIs (reporting non-portable or internal runtime class) so we @SuppressForbidden liberally in these fixtures and I cannot see any obvious way to indicate that HttpServer#createContext is really forbidden, unlike the rest of this stuff that's ok in these contexts.

Can we make Forbidden-APIs less sensitive to the use of these internal classes in a test context so that we can drop most of the @SuppressForbidden annotations? Any other ideas?

Relates #68957

[elasticmachine]

Pinging @elastic/es-delivery (Team:Delivery)

[mark-vieira]

One option is to simply define a class in our test framework that simply extends HttpServer and exclusively use that. We could then have our TestHttpServer (or whatever we call it) register our wrapper in a constructor or other overridden methods and that would allow us to scope the usage of @SuppressForbidden just to that class itself.

[DaveCTurner]

I just love how you used the word "simply" twice in that first sentence 😁

Yeah that's an option that I tried, but it's not simple, there are a bunch of forbidden dependent classes we'd also need to wrap: HttpExchange, HttpHandler, Headers, HttpsServer, HttpsParameters, HttpsExchange, HttpsConfigurator, maybe more (that was the point at which I gave up and opened this issue).

[mark-vieira]

Yeah, this is annoying. There doesn't seem to be anyway to do precision overrides like this. Our usage of the non-portable bundled signature just blanketly disallows all the com.sun stuff. The scope of this bundled signature is quite large:

jdk-non-portable: Signatures of all non-portable (like com.sun.management.HotSpotDiagnosticMXBean) or internal runtime APIs (like sun.misc.Unsafe). This is a superset of jdk-internal.
Internally this is implemented using heuristics: Any reference to an API that is part of the Java runtime (rt.jar, extensions, Java 9+ java.* / jdk.* core modules) and is not part of the Java SE specification packages (mainly java, javax, but also org.ietf.jgss, org.omg, org.w3c.dom, and org.xml.sax) is forbidden (any java version, no specific JDK version, since forbiddenapis v2.1).

Well, there's loads of "public" and "documented" APIs that are not part of the Java SE API to include jdk.httpserver. It would seem under the same rule using the compiler API would also not be allowed 🤦.

One option we potentially have here is to remove the usage of jdk-non-portable and replace it with something more targeted to our intention.

[DaveCTurner]

Can we use different signature bundles in test code vs prod? I think it's right to avoid this stuff in production, but IMO we can be more liberal with tests since we control the environment and breakage only affects us internally.

I've no idea whether this bundle covers anything sufficiently dangerous that we'd want to avoid it in tests too.

[mark-vieira]

Can we use different signature bundles in test code vs prod?

We can. Right now we configured them identically but we could relax the constraints on test code. I'm not aware of all the potential side-effects of this. There's likely historical precedent for adding these checks to begin with.

[DaveCTurner]

Ok that sounds like a good direction to me then, thanks. I'm not sure who owns the question of whether it's acceptable to relax these checks, my best guess would be @elastic/es-core-infra so I'll mark this for their discussion.

---

Summary

We use com.sun.net.httpserver.* fairly extensively in tests, but these classes are currently forbidden APIs according to the jdk-non-portable signature bundle so we have to sprinkle @SuppressForbidden everywhere. I think we should really forbid some of the APIs exposed in that package, but if everything is forbidden then nothing is.

I would like to relax the forbidden API signature bundle from jdk-non-portable to something else, I guess jdk-unsafe and jdk-internal, for enough of our test code that we don't need to @SuppressForbidden our usages of com.sun.net.httpserver.*.

I have tried to determine the precise impact of this proposal without much success: forbiddenapis determines non-portability programmatically so I don't see an easy way to get a list of everything we would end up permitting. But still, we're talking about test code alone: we control the environments in which it runs, and if it breaks then it only hurts us.

[mark-vieira]

I'm not sure who owns the question of relaxing these checks

It would be a build change so I think delivery would still own making the actual change. I do think whether or now we should make the change though would be a larger discussion.

[DaveCTurner]

acked, yes, reworded slightly:

I'm not sure who owns the question of whether it's acceptable to relax these checks

[rjernst]

We already have separate signature files for test vs server (as well as "all" that applies to both). It would be trivial to move the bundled signatures to this condition.

However, I want to first understand a little more what it is we are talking about:

for writing test fixtures

Do you mean in memory fixtures for unit tests, or the test fixtures we build in gradle to be used with integration tests? If the latter, we shouldn't need to run forbidden apis on the fixture at all (and I thought we didn't). If the former, jdk.httpserver seems like the better choice long term, but in the meantime having a centralized utility for this seems useful. We already use MockSocket for building these in the rest client tests.

[DaveCTurner]

Do you mean in memory fixtures for unit tests, or the test fixtures we build in gradle to be used with integration tests?

We use these APIs both in standalone fixtures (e.g. test/fixtures/s3-fixture/src/main/java/fixture/s3/S3HttpFixture.java) and in unit tests (e.g. plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureBlobContainerRetriesTests.java). I would like us to be using forbidden APIs in both cases, because I want to forbid HttpServer#createContext(String, HttpHandler) since it quietly catches Throwable.

jdk.httpserver seems like the better choice long term

I am not clear what jdk.httpserver refers to here - I interpret it as the jdk.httpserver module built into the JDK, including all the com.sun.net.httpserver.* classes that we're already using, and therefore I'm misunderstanding your comment somehow.

[rjernst]

The confusion was on my end, I misunderstood a comment above and referring to jdk.httpserver thinking it was a future official Java API, but now realize it is just the module that contains the existing HttpServer.

Going back to the original issue, as long as we keep jdk-non-portable for the non-test configuration, I think your suggestion (using jdk-unsafe and jdk-internal as suggested here) is fine.