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

NettyClientTransport: use getOrCreate() for new instances of ChannelLogger AttributeKey. #7048

Merged
merged 10 commits into from May 21, 2020

Conversation

Nexproc
Copy link
Contributor

@Nexproc Nexproc commented May 19, 2020

NettyClientTransport throws an error when the class is loaded in two separate class-loaders because of some implementation detail with the underlying AttributeKey class. Instead of just blowing up when a new ChannelLogger key can't be created, this allows multiple, isolated class-loaders in the same JVM to just get the existing key. If this causes issues, this AttributeKey should be updated to use a new UUID (or something) each time it is loaded.

Improvement: Works with isolated class-loaders.
Potential Drawback: Uses the same ChannelLogger AttributeKey object across multiple, isolated classloaders.

Fixes #6707

voidzcy and others added 2 commits May 19, 2020
…hannelLogger` `AttributeKey`.

`NettyClientTransport` throws an error when the class is loaded in two separate class-loaders because of some implementation detail with the underlying `AttributeKey` class. Instead of just blowing up when a new `ChannelLogger` key can't be created, this allows multiple, isolated class-loaders in the same JVM to just get the existing key.  If this causes issues, this `AttributeKey` should be updated to use a new UUID (or something) each time it is loaded.

Improvement: Works with isolated class-loaders.
Potential Drawback: Uses the same `ChannelLogger` `AttributeKey` object across multiple, isolated classloaders.
@linux-foundation-easycla
Copy link

@linux-foundation-easycla linux-foundation-easycla bot commented May 19, 2020

CLA Check
The committers are authorized under a signed CLA.

@ejona86
Copy link
Member

@ejona86 ejona86 commented May 19, 2020

@Nexproc, please fix the checkstyle failures reported by Travis.

@ejona86 ejona86 added the TODO:release blocker label May 19, 2020
@ejona86
Copy link
Member

@ejona86 ejona86 commented May 19, 2020

As I mention on #6707, it is probably better to delete this attribute. But I think it would be good to merge this mostly as-is for the v1.30 release and then do the nicer cleanup separately.

@ejona86 ejona86 self-requested a review May 19, 2020
@voidzcy voidzcy added this to the 1.30 milestone May 20, 2020
@voidzcy voidzcy added the TODO:backport label May 20, 2020
Nexproc added 8 commits May 21, 2020
reorder `final static` -> `static final`
Shortened the null check to get around coverage being upset about 1 line. This lets me avoid making this getOrCreate function public just to test whether AttributeKey works.
@ejona86 ejona86 added the kokoro:run label May 21, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run label May 21, 2020
@ejona86 ejona86 requested a review from creamsoup May 21, 2020
@creamsoup creamsoup merged commit 3601190 into grpc:master May 21, 2020
12 of 13 checks passed
@creamsoup
Copy link
Contributor

@creamsoup creamsoup commented May 21, 2020

merged, thanks @Nexproc

@Nexproc
Copy link
Contributor Author

@Nexproc Nexproc commented May 21, 2020

No problem! Thank you both, @creamsoup & @ejona86, for the quick turnaround!

@ejona86
Copy link
Member

@ejona86 ejona86 commented May 21, 2020

Since I realized this does not impact netty+netty-shaded, this is no longer a release blocker.

@ejona86 ejona86 removed the TODO:release blocker label May 21, 2020
@creamsoup creamsoup removed the TODO:backport label May 21, 2020
dfawley pushed a commit to dfawley/grpc-java that referenced this issue Jan 15, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants