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
Add support for anonymous in-process servers. #8589
Changes from 4 commits
babfb2b
28fc525
361c9f8
df89d02
026113a
ad8adb3
619902f
506b46e
4ac409d
4d5a1e3
fc61151
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,11 +40,20 @@ final class InProcessServer implements InternalServer { | |
private static final ConcurrentMap<String, InProcessServer> registry | ||
= new ConcurrentHashMap<>(); | ||
|
||
static InProcessServer findServer(String name) { | ||
return registry.get(name); | ||
static InProcessServer findServer(SocketAddress addr) { | ||
if (addr instanceof InProcessSocketAddress) { | ||
InProcessSocketAddress inProcessAddress = (InProcessSocketAddress) addr; | ||
if (inProcessAddress.getServer() != null) { | ||
return inProcessAddress.getServer(); | ||
} else { | ||
return registry.get(inProcessAddress.getName()); | ||
} | ||
} | ||
return null; | ||
} | ||
|
||
private final String name; | ||
private final boolean anonymous; | ||
private final int maxInboundMetadataSize; | ||
private final List<ServerStreamTracer.Factory> streamTracerFactories; | ||
private ServerListener listener; | ||
|
@@ -61,6 +70,7 @@ static InProcessServer findServer(String name) { | |
InProcessServerBuilder builder, | ||
List<? extends ServerStreamTracer.Factory> streamTracerFactories) { | ||
this.name = builder.name; | ||
this.anonymous = builder.anonymous; | ||
this.schedulerPool = builder.schedulerPool; | ||
this.maxInboundMetadataSize = builder.maxInboundMetadataSize; | ||
this.streamTracerFactories = | ||
|
@@ -71,15 +81,17 @@ static InProcessServer findServer(String name) { | |
public void start(ServerListener serverListener) throws IOException { | ||
this.listener = serverListener; | ||
this.scheduler = schedulerPool.getObject(); | ||
// Must be last, as channels can start connecting after this point. | ||
if (registry.putIfAbsent(name, this) != null) { | ||
throw new IOException("name already registered: " + name); | ||
if (!anonymous) { | ||
// Must be last, as channels can start connecting after this point. | ||
if (registry.putIfAbsent(name, this) != null) { | ||
throw new IOException("name already registered: " + name); | ||
} | ||
} | ||
markb74 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
@Override | ||
public SocketAddress getListenSocketAddress() { | ||
return new InProcessSocketAddress(name); | ||
return new InProcessSocketAddress(name, anonymous ? this : null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This choice is notable. The address instance will only reference the server if it's anonymous. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think that is the correct behavior. At first I was like, "oh, definitely," then I realized "but that might produce weird behavior/semantics" because depending on how the address was created you'd get different behavior. But I actually think it works well. Essentially it is the same as the anonymous server having a randomly-generated ID that is never reused.
ejona86 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
@Override | ||
|
@@ -99,8 +111,10 @@ public List<InternalInstrumented<SocketStats>> getListenSocketStatsList() { | |
|
||
@Override | ||
public void shutdown() { | ||
if (!registry.remove(name, this)) { | ||
throw new AssertionError(); | ||
if (!anonymous) { | ||
if (!registry.remove(name, this)) { | ||
throw new AssertionError(); | ||
} | ||
} | ||
scheduler = schedulerPool.returnObject(scheduler); | ||
synchronized (this) { | ||
|
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 possibility of creating a base AbstractInProcessSocketAddress class with package visible findServer(), registerServer(InProcessServer) and unregisterServer(InProcessServer) methods occurred to me.
That would allow this method, and registerInstance() and unregisterInstance() below to just be methods on the address, removing the need for instance checks.
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.
Optimizing this method seems like a micro-optimization. If this method is the only one that benefits, then avoiding public API seems clearly superior. If we think user's code would improve, then common base class is more interesting. Seems we can always do it in the future, so let's favor less API for now.
Your suggestion would also mean moving the registry to InProcessSocketAddress, which propagates the "strangeness" of anonymous socket address. Seems counter to our preferences.
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.
Makes sense. I was attracted to the approach making everything more consistent, even if that does mean "embracing the strangeness" to some degree. But I like philosophy that improving user code is the key goal, so agreed this probably isn't worth it.