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

Don't await a StackOverflowError on a redirection loop #4452

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thahnen
Copy link
Contributor

@thahnen thahnen commented Nov 26, 2024

In case of a redirection loop for URI of a p2 repository, fail fast on a redirection loop. In this case provide a warning to the user and let the rest be handled by Tycho. Otherwise there might be a StackOverflowError due to the recursion used and no check for whether the base URI is the same as the redirected URI.

This links to #4451 but does not fix it, only raising visibility for such cases.

In case of a redirection loop for URI of a p2 repository, fail fast on a redirection loop. In this case provide a warning to the user and let the rest be handled by Tycho.
Otherwise there might be a StackOverflowError due to the recursion used and no check for whether the base URI is the same as the redirected URI.

This links to eclipse-tycho#4451 but does not fix it, only raising visibility for such cases.
Copy link

github-actions bot commented Nov 26, 2024

Test Results

  603 files    603 suites   4h 22m 4s ⏱️
  431 tests   424 ✅  7 💤 0 ❌
1 293 runs  1 271 ✅ 22 💤 0 ❌

Results for commit 9fa7feb.

♻️ This comment has been updated with latest results.

@laeubi
Copy link
Member

laeubi commented Nov 26, 2024

Any chance you can enhance the existing integration test to show the behavior?

@thahnen
Copy link
Contributor Author

thahnen commented Nov 26, 2024

@laeubi which one is the corresponding one?

I was thinking about creating a new one with a web server redirecting to itself to mimic this. But I haven't finished yet reading the documentation of Jetty ^^

@laeubi
Copy link
Member

laeubi commented Nov 26, 2024

If you search for HttpServer references in the test you should find some test that already do something in that direction.

@thahnen
Copy link
Contributor Author

thahnen commented Nov 27, 2024

Hi @laeubi, sorry for the late response. I will take some time later today to create an IT or to update a fitting existing one.

@thahnen
Copy link
Contributor Author

thahnen commented Nov 27, 2024

Sadly, for whatever reason I wasn't able to run the ITs from within Eclipse itself due to the launch configuration tycho-its - prepare test resources failing with an exception from both the launcher as well as when running the test from the command line mvn clean verify -f tycho-its/pom.xml -Dtest=P2RedirectLoopTest:
prepare_test_resources.log

I have to await the CI and then wait and see.

@laeubi
Copy link
Member

laeubi commented Nov 28, 2024

In eclipse you can simpel right click and choose to run as unit test. But due to the nature of tycho/maven it is usually better to use maven CLI with remote debugging.

For this simply do

  • mvn clean install -T1C -DskipTests
  • cd tycho-its
  • mvn clean install -Pits -Dtest=<full class name of test>

@thahnen
Copy link
Contributor Author

thahnen commented Dec 2, 2024

Hey @laeubi,

I have been trying for a few days now to create an IT for this behavior but with no success. Taking a look at all P2-related ITs, I could not really find a way to hook into one, so I created one on my own.

But even having a Jetty where the p2.index was reachable and linked to a compositeContent.xml and compositeArtifacts.xml on another server that would create the redirection loop did not work.
My initial idea of just having a target platform linking to a server that would redirect to itself for every file would not work because it failed earlier by not finding a p2 metadata repository 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants