Handle multiple gRPC port registrations#54928
Conversation
Status for workflow
|
| int actualPort = grpcServer.getPort(); | ||
| valueRegistry.getValue().register(GRPC_PORT, actualPort); | ||
| if (launchMode.isDevOrTest()) { | ||
| valueRegistry.getValue().register(GRPC_TEST_PORT, actualPort); |
There was a problem hiding this comment.
There is a check-and-act race condition here. You check that it's not set, and set it afterwards, without any check that it was not set in between by another thread.
It might be an API issue as it would be better to have something like: registerIfAbsent, or something like registerIfAbsent that takes a Supplier and guarantees that if would not be changed in between.
There was a problem hiding this comment.
Alternatively, and much simpler: registerPort.compareAndSet(false, true)
There was a problem hiding this comment.
Alternatively, and much simpler: registerPort.compareAndSet(false, true)
Where?
Since that is exactly what @radcortez said it should not be done.
I think you're right, the ValueRegistry should have the API as you said -- registerIfAbsent.
Anything else can have a race or set things before they actually work; e.g. exception is thrown, but we already set it to true.
There was a problem hiding this comment.
@radcortez what's the rationale for not doing a compareAndSet?
There was a problem hiding this comment.
Because io.grpc.Server#getPort may return IllegalStateException if the server is not yet started.
We could do compareAndSet and reset it in the case if throw an exception.
3 participants
Ignore next port registrations for multi-instance gRPC usage.