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 - cache local and remote socket address in EndPoint for benefit of RequestLog #10815

Closed
wants to merge 3 commits into from

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Oct 31, 2023

The EndPoint local-address and remote-address are inaccessible once the EndPoint is closed due to "ensureOpen" checks by the Java implementation itself.

This PR merely caches the local-address and remote-address in the EndPoint classes to be accessed if unable to get the attempt to get the SocketAddress from the Channel fails due to a ClosedChannelException.

Fixes: #10806

@joakime joakime added the Bug For general bugs on Jetty side label Oct 31, 2023
@joakime joakime added this to the 12.0.x milestone Oct 31, 2023
@joakime joakime requested review from gregw and sbordet October 31, 2023 16:40
@joakime joakime self-assigned this Oct 31, 2023
@joakime joakime linked an issue Oct 31, 2023 that may be closed by this pull request
gregw
gregw previously requested changes Nov 1, 2023
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.

Amazed we have not noticed this before!

@joakime joakime requested a review from gregw November 1, 2023 22:00
@gregw
Copy link
Contributor

gregw commented Nov 1, 2023

@joakime I've pushed a bit more adventurous cleanup of local/remote addresses to this PR. Thoughts?

@gregw gregw dismissed their stale review November 1, 2023 23:51

I've contributed now

@joakime
Copy link
Contributor Author

joakime commented Nov 2, 2023

@joakime I've pushed a bit more adventurous cleanup of local/remote addresses to this PR. Thoughts?

Interesting, kinda like it.
How well does the UnixDomainSocket stuff work with this new approach?

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.

I don't think this is right.
What about the case of socket address reuse?

If it's just a problem of the request log, it probably should be fixed there rather than returning cached addresses that may be used in completely different contexts where returning a cached address is not right.

For example clients may behave differently from servers.

Also, in QUIC the remote address may change dynamically, so we don't want to cache.

The RequestLog is passed _request.getLoggedRequest(), so we can easily wrap the "logged" request and cache the socket addresses there, rather than everywhere else.

@joakime
Copy link
Contributor Author

joakime commented Nov 6, 2023

If it's just a problem of the request log, it probably should be fixed there rather than returning cached addresses that may be used in completely different contexts where returning a cached address is not right.

The problem is way down in the Java Impl.
If the socket is closed, you cannot get the LocalAddress or RemoteAddress out from it due to an ensureOpen() check in the implementation.

@joakime
Copy link
Contributor Author

joakime commented Nov 6, 2023

I'm working on an alternative approach suggested by a conversation with @sbordet, i'll submit that as a new PR.

@gregw
Copy link
Contributor

gregw commented Nov 8, 2023

I think there might be some goodness in the cleanup in this PR even if we don't make addresses immutable in end points. So, before closing this one, please look to see if any of the changes make sense if we don't make the addresses immutable.

@joakime
Copy link
Contributor Author

joakime commented Nov 9, 2023

Dropping in favor of #10867

@joakime joakime closed this Nov 9, 2023
@gregw gregw deleted the fix/12.0.x/requestlog-remoteaddr branch December 12, 2023 20:22
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