Remove testRequestHeaderTooLarge()

[dimas-b]

This test is not normative to Polaris APIs.

Moreover, it does not test any of Polaris code, but instead depends on Dropwizard's behaviour.

While the intent of making sure that header sizes are restricted is wise, it does not look like validating that in Polaris CI is the best option. As the comment in the deleted test suggests actual runtime behaviour is heavily affected by the execution environment.

[flyrain]

LGTM.

[eric-maynard]

I'm a little wary of removing this test. Is there a scenario (with Quarkus?) where it fails?

[dimas-b]

@andrew4699 : WDYT?

[dimas-b]

@eric-maynard : "fails" is what sense? Do you mean Quarkus might not have restrictions on header sizes?... or that Quarkus default limits might be too large and cause issues in Polaris code?

[dimas-b]

After this discussion, I think I'll keep the test, but make it check only the status code, without checking the response payload since some systems may not provide the payload.

[dimas-b]

Ok, pivoting again (per discussion under #631). Let's remove both the "body" and "header" (this PR) size tests.

[eric-maynard]

"fails" is what sense?

In the sense of the test literally failing. Are you removing this test because it will fail after the Quarkus refactor?

[andrew4699]

From the other thread:

I personally do not see much value in a test that either validates the status code or does not validate anything because a connection close exception is inconclusive and does not guarantee that even the request was sent.

Connection close is broad but if we have very similar code elsewhere and the connection only closes when we set large headers, that's pretty strong evidence that the large headers caused that.

This test is not normative to Polaris APIs.

Moreover, it does not test any of Polaris code, but instead depends on Dropwizard's behaviour.

Sure but isn't that most things? Polaris tests should test that we've made the right choices when integrating with the libraries we use.

IMO there's value in keeping this test. It decreases the likelihood that we accidentally flip a switch or migrate to a framework that disables this protection.

[dimas-b]

I'll do some debugging...

[dimas-b]

Are you removing this test because it will fail after the Quarkus refactor?

No. Even with DW the "header" test expects connection closed exceptions. It's this uncertainty of behaviour that makes the test not so valuable, IMHO. If the test gives a false positive result (by getting a connection closed exception for another reason), it's worse than having no test, IMHO.

[eric-maynard]

I don't think the fact that the test could (but has never been observed to?) sometimes flake and give false positives is a compelling enough reason to remove it without a replacement.

Moreover, it does not test any of Polaris code, but instead depends on Dropwizard's behaviour.

It tests the Polaris application.

[dimas-b]

It tests the Polaris application.

True, but is it "Polaris" that specifies the behaviour of handling excessively large headers? In this particular case, it looks like confounding influence from infrastructural components of the "application" is so great as to make a deterministic test impractical.

That said, I'll try and debug whether the connection closed problems might be a client-side thing.

[adutra]

Another suggestion: maybe in the spirit of moving forward quickly with #590, let's keep the tests and add a catch clause for connection closed exceptions in both tests. How does that sound?

[dimas-b]

Converted this PR to "draft" pending in-depth investigation.

[dimas-b]

I believe the connection reset happens if/when the request is split across two (or more) TCP packets. The server responds having read the first packet and closes the connection. The client attempts to keep writing the rest of the request and gets a connection closed exception (specifically SocketException: Broken pipe under Dropwizard).

I'd argue that the client in use by this test is not robust enough for this test case.

We probably need a different client impl. to gracefully handle reading the server's status line in this case even though the full request could not be written.

[dimas-b]

Test robustness was improved in #590... Closing this PR.

[dimas-b]

Out of curiosity I ran the same test with HttpURLConnection and it was able to obtain the status code in thousands of requests, while the existing client failed with a "connection reset" after about 15 requests... but even the HttpURLConnection was not 100% reliable, so I went with Awaitility in #590.