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

Fixes #10806 - Introduce RequestLogWrapper to address SocketAddress concerns #10861

Closed

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Nov 6, 2023

Alternate approach from PR #10815

Uses a RequestLogWrapper to address requestlog concerns, instead of modifying the low level IO layer.

Fixes #10806

@joakime joakime added the Bug For general bugs on Jetty side label Nov 6, 2023
@joakime joakime requested review from gregw and sbordet November 6, 2023 20:36
@joakime joakime self-assigned this Nov 6, 2023
@joakime joakime added this to the 12.0.x milestone Nov 6, 2023
@joakime joakime linked an issue Nov 6, 2023 that may be closed by this pull request
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

LGTM beside the nit about Awaitility.
Much simpler solution than the other PR.

// Close connection
endPoint.close();
// Wait for endpoint to be closed
Awaitility.await().atMost(Duration.ofSeconds(5)).until(() -> !endPoint.isOpen());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please static import Awaitility.await like we do with other testing facilities.

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

I think I like the other approach more. I don't like putting on another wrapper just to remember address data that might go away.

I prefer the approach that an end point has local/remote address regardless of it's open/close state. In this case we have been caught by the request log, but I can imagine other situations (specially debugging) that we are racing with a close to get the addresses.

Far better to make the the addresses on an endpoint immutable rather than always wrap them in case they are not.

@gregw
Copy link
Contributor

gregw commented Nov 8, 2023

@sbordet @joakime I now agree that #10815 is wrong to fix the remote/local address in the EndPoint, as that is intrinsically dynamic.

This PR is correct in that the solution is to make the addresses immutable for the duration of a request cycle. But it is doing that wrong, as it is replacing a probably immutable ConnectionMetaData with a definitely immutable one. So it is often wasted effort.

I think the correct solution is that ConnectionMetaData should always be immutable. I.E any Connection that provides a ConnectionMetaData for a request cycle should provide a non-dynamic instance - so it will ask the endpoint at request init time what the addresses are. That ConnectionMetaData will then never change. The Connection can then decide if it reuses it for subsequent requests or creates a new one for every request.

@gregw
Copy link
Contributor

gregw commented Nov 8, 2023

Note that the ConnectionMetaData is passed to the constructor of the HttpChannel, so that any connection type that does support changing addresses for remote/local will need to recreate the HttpChannel instance.
So the change here is that h3 will need to remake the ConnectionMetaData and HttpChannel's whenever it detects an address has changed (or maybe on every request cycle, but that could be slow).

@sbordet are h2/h3 recycling HttpChannel instances at the moment?

@lorban at some stage we need to evaluate if recycling HttpChannels is worth it.... we can probably test that easily in HTTP/1, which does reuse, by making a version that does not.

@gregw
Copy link
Contributor

gregw commented Nov 9, 2023

@joakime I've implemented my suggested approach, of making the addresses immutable within the ConnectionMetaData, which in practise for h1, h2 & FCGI is in the Connection, but for h3 is a new ConnectionMetaData in each onRequest call.

Note that this also revealed that only h2 was correctly respecting HttpConfiguration.getLocalAddress(). I have now implemented for h1, h2, h3 & FCGI.

@joakime
Copy link
Contributor Author

joakime commented Nov 9, 2023

Dropping in favor of #10867

@joakime joakime closed this Nov 9, 2023
@joakime joakime deleted the fix/12.0.x/requestlog-wrapped-remoteaddr branch December 20, 2023 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Can't always access remote address from RequestLog
3 participants