Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
8350279: HttpClient: Add a new HttpResponse method to identify connections #24154
base: master
Are you sure you want to change the base?
8350279: HttpClient: Add a new HttpResponse method to identify connections #24154
Changes from 8 commits
c30d5bb
7cde035
8f63c22
9765c08
4c5d559
5c57d05
f8b0de5
2857dde
028999a
31a5647
a08c0cf
59a11cc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the API documentation on
HttpResponse.connectionLabel()
we talk about the connection label being unique within aHttpClient
scope. i.e. two differentHttpClient
instances could have the same connectionLabel for a connection. I think that's the right scoping.So given that, having a
static
field on aHttpConnection
which keeps track of a connection label, may not be the right place to keep track of that state. In this proposed form, no two connections in two different HttpClient instances will ever have the same connectionLabel. I think this counter should probably be present on theHttpClientImpl
as an instance field.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that's OK. We could go either way - but I'd rather keep the label unique regardless of the client because otherwise we would also need to include the client id in the debug traces (ATM we only have the label). If it's unique in the scope of the VM it's a fortiori unique in the scope of the client instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Volkan, I think we shouldn't talk about any kind of ordering here. I see that the
id
field has been updated with a comment stating that it provides ordering among instances. Even there, I don't think that comment about ordering is needed. So far we haven't used theid
to mean anything other than a logical identifier and these values have played no role in ordering of connections at any place. So adding the comments about ordering, I think, makes it confusing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only use of
HttpConnection.id
is for ordering - so that connections can be placed in a ConcurrentSkipListSet. So maybe here we should not speak of ordering (remove the last sentence in that paragraph) - but I think it helps to keep it forid
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware of that. Now that you mentioned it, I looked up the code which uses the Set to store these connections. And from what I can see, the order is only used during the closing of a HttpClient, to close these opened connections in that specific order. Did I miss any other usages of the order?
In any case, now that you corrected me about the usage of the
id
order, I agree that having the comment on theid
field is fine and only remove it from the paragraph here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we close a connection, we take it out of the set. So it's not about ordering the connection but about quickly finding the connection in that set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be
private
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps make this method
final
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not. We could want to extend it to give an indication that the connection is TCP or TLS - or Quic in the future...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copyright year on this file will need an update.