Skip to content

Conversation

@zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Nov 14, 2025

We broke native image and didn't realize it because

  • main was broken at the time due to spring 7 latest deps
  • it took more that 1 PR to fix latest deps and thus didn't see the failure

@zeitlinger zeitlinger self-assigned this Nov 14, 2025
@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Nov 14, 2025
@zeitlinger zeitlinger marked this pull request as ready for review November 14, 2025 20:38
@zeitlinger zeitlinger requested a review from a team as a code owner November 14, 2025 20:38
@zeitlinger zeitlinger added this to the v2.22.0 milestone Nov 14, 2025
private static MethodHandle findGetHeadersMethod(MethodType methodType) {
try {
return MethodHandles.lookup().findVirtual(HttpHeaders.class, "getOrDefault", methodType);
return MethodHandles.lookup().findVirtual(HttpHeaders.class, "get", methodType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be interesting to know why getOrDefault failed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I don't have an answer

// up to spring web 7.0
methodHandle =
findGetHeadersMethod(MethodType.methodType(Object.class, Object.class, Object.class));
methodHandle = findGetHeadersMethod(MethodType.methodType(List.class, Object.class));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be Object.class, Object.class

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laurit
Copy link
Contributor

laurit commented Nov 15, 2025

I created #15311 that uses a slightly different approach just to test out whether it would work. Noticed that for webflux instrumentation the header getter logic is duplicated for client and server instrumentation, fixed that too. The approach taken in this PR is fine too and I might even prefer it. Though first all tests would need to pass.

@zeitlinger
Copy link
Member Author

I created #15311 that uses a slightly different approach just to test out whether it would work. Noticed that for webflux instrumentation the header getter logic is duplicated for client and server instrumentation, fixed that too. The approach taken in this PR is fine too and I might even prefer it. Though first all tests would need to pass.

great - I'm also trying to fix this PR so that we can choose.

  • I've also opted not to ignore exceptions (which has already made it hard to fix the PR)
  • added explicit check for spring 7 instead
  • not removed duplication yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test native This label can be applied to PRs to trigger them to run native tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants