Add timeout to gathering host certificates #567

[danthe1st]

As explained in #567, it is currently possible that Eclipse waits infinitely for a host certificate in case the server accepting the request but not responding after that. This issue caused the plugin installation of a Stack Overflow user to get stuck.

This PR adds a timeout and retrying using the properties org.eclipse.equinox.p2.engine.requestTimeout and org.eclipse.equinox.p2.engine.requestRetries.

How to test

• Create a server that accepts incoming connections but never responds, e.g. nc -l 8080 -k on Linux or using Java code similar to the following (not using TLS for simplicity):

  try(ServerSocket servSock = new ServerSocket(8080)){
      while(true){
          try(Socket sock = servSock.accept();
              BufferedReader br = new BufferedReader(new InputStreamReader(sock.getInputStream()))){
              br.lines().forEach(System.out::println);
          }
      }
  }

• In the Eclipse application, go to Preferences > Install/Update > Trust > Authorities
• Change http to Allow
• Click Add
• Enter http://localhost:8080

Without this change, Eclipse sends one HTTP request to this server, never closes the request and the thread is blocked forever.

With this change, it sends multiple (by default 3) requests to the server and then skips the host.

Other changes

I also added a warning log when gathering certificates throws an exception. I made sure the current thread is reinterrupted when an InterruptedException is thrown (and not logging anything since that seems unnecessary).

Notes

Since I didn't find a property that I felt was actually applicable to reuse for the timeout and retry count, I created new ones. I decided on a default of 2 retries (i.e. 3 requests in total) with a timeout (not the connection timeout but the timeout of the request, i.e. when waiting for a response after connecting) of 5 seconds. I didn't include a backoff time between requests but I could still add that if wanted.

[danthe1st]

I hope the way I updated the versions in the MANIFEST.MF and feature.xml files is correct (I haven't contributed code to Eclipse repos before).

[github-actions]

Test Results

  250 files   -   125    250 suites   - 125   27m 24s ⏱️ - 19m 37s
1 898 tests  -     6  1 895 ✅  -     6  3 💤 ±0  0 ❌ ±0 
4 478 runs   - 2 234  4 472 ✅  - 2 231  6 💤  - 3  0 ❌ ±0 

Results for commit dc2d50b. ± Comparison against base commit b4a50b8.

This pull request removes 6 tests.

AutomatedTests org.eclipse.equinox.p2.tests.touchpoint.natives.AllTests org.eclipse.equinox.p2.tests.touchpoint.natives.CheckAndPromptNativePackageWindowsRegistryTest ‑ execute_IntAttribute
AutomatedTests org.eclipse.equinox.p2.tests.touchpoint.natives.AllTests org.eclipse.equinox.p2.tests.touchpoint.natives.CheckAndPromptNativePackageWindowsRegistryTest ‑ execute_IntAttribute_DifferentLiteralValue
AutomatedTests org.eclipse.equinox.p2.tests.touchpoint.natives.AllTests org.eclipse.equinox.p2.tests.touchpoint.natives.CheckAndPromptNativePackageWindowsRegistryTest ‑ execute_KeyExistence
AutomatedTests org.eclipse.equinox.p2.tests.touchpoint.natives.AllTests org.eclipse.equinox.p2.tests.touchpoint.natives.CheckAndPromptNativePackageWindowsRegistryTest ‑ execute_KeyExistence_DifferentKeys
AutomatedTests org.eclipse.equinox.p2.tests.touchpoint.natives.AllTests org.eclipse.equinox.p2.tests.touchpoint.natives.CheckAndPromptNativePackageWindowsRegistryTest ‑ execute_StringAttribute
AutomatedTests org.eclipse.equinox.p2.tests.touchpoint.natives.AllTests org.eclipse.equinox.p2.tests.touchpoint.natives.CheckAndPromptNativePackageWindowsRegistryTest ‑ execute_StringAttribute_DifferentValues

♻️ This comment has been updated with latest results.

[merks]

I can confirm that PR behaves described:

You've done a good job figuring out what's needed in terms of version increments for manifests and features.

[merks]

Overall it's nice work and much appreciated! Just a few very tiny nits and I think we're good to go.

[danthe1st]

Overall it's nice work and much appreciated! Just a few very tiny nits and I think we're good to go.

I have committed the requested changes. I assume the GitHub Actions failure (Component 'Provisioning Admin UI RCP (Incubation)' in the baseline 'workspace' is disposed [main]) on Windows before is unrelated to my changes?

[merks]

Looks good now. Could you please squash this into a single commit and force push it?

[danthe1st]

I made the requested changes (I had to bump the minor version of one bundle due to adding IProvisioningAgent#getIntProperty which is an API change) and squashed it.
The diff of my changes now (compared to the state of the last reviews) should be https://github.com/eclipse-equinox/p2/compare/23da223b7612e835266b7e063cae2292d67f98ce..4f6d358a86b1cb4644233cb96fe4ba665d1d1b4e

[merks]

Only the version of bundles/org.eclipse.equinox.p2.core/META-INF/MANIFEST.MF should be fixed. The rest is at your discretion.

Thanks for being so super responsive! You're awesome!!

[merks]

When the build passes, it's ready to merge. 🥇

[merks]

Thanks again!! ❤️