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

Immutable local/remote SocketAddress within a ConnectionMetaData #10867

Merged

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Nov 9, 2023

The local/remote SocketAddress is cached within the ConnectionMetaData or Connection instance, so that any changes are not visible during the request lifetime.
Ensure that all server Connection types respect HttpConfiguration#getLocalAddress

This is an alternate to both #10861 and #10815 to fix #10806

Co-authored-by: Joakim Erdfelt joakim.erdfelt@gmail.com

The local/remote SocketAddress is cached within the ConnectionMetaData or Connection instance, so that any changes are not visible during the request lifetime.
Ensure that all server Connection types respect HttpConfiguration#getLocalAddress
@gregw gregw linked an issue Nov 9, 2023 that may be closed by this pull request
@gregw
Copy link
Contributor Author

gregw commented Nov 9, 2023

@joakime Can you remind me why HttpConfiguration#getLocalAddress was not implemented as a request Customizer?

Could we change the implementation so that if a non null address is set on the HttpConfiguration, then that is equivalent to adding a customizer that will force the address.
We can then remove all the handling in the connections for this.

The local/remote SocketAddress is cached within the ConnectionMetaData or Connection instance, so that any changes are not visible during the request lifetime.
Ensure that all server Connection types respect HttpConfiguration#getLocalAddress
@gregw gregw requested review from sbordet and joakime November 9, 2023 10:41
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

Just questions about the returning of null and storing of null in final fields.
And seemingly missing handling of HttpConfiguration.(local)Address.

The local/remote SocketAddress is cached within the ConnectionMetaData or Connection instance, so that any changes are not visible during the request lifetime.
Ensure that all server Connection types respect HttpConfiguration#getLocalAddress and that it is not implemented only in servlet layer
Avoid DNS resolution.
@gregw
Copy link
Contributor Author

gregw commented Nov 9, 2023

@joakime I've pushed a few more commits dealing with HttpConfiguration.getLocalAddress(). We were dealing with that in servlet land... which was fine when we didn't have a core request. Now that it is dealt with at the connection level, we can remove all that handling from the servlet implementation(s).

joakime
joakime previously approved these changes Nov 10, 2023
@@ -46,6 +46,21 @@ public DatagramChannel getChannel()
return (DatagramChannel)super.getChannel();
}

@Override
public SocketAddress getRemoteSocketAddress()
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be moved to SelectableChannelEndPoint where getLocalSocketAddress() is.
And be removed also from SocketChannelEndPoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can't as the getRemoteAddress on a DatagramChannel is a different method than the one on SocketChannel and does not exist in NetworkChannel.

There is a reason it is as it is.

The local/remote SocketAddress is cached within the ConnectionMetaData or Connection instance, so that any changes are not visible during the request lifetime.
Ensure that all server Connection types respect HttpConfiguration#getLocalAddress and that it is not implemented only in servlet layer
Avoid DNS resolution.
@gregw gregw merged commit 2c35f5a into jetty-12.0.x Nov 13, 2023
9 checks passed
@gregw gregw deleted the fix/jetty-12.0.x/immutableAddressesInConnectionMetaData branch November 13, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
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