Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
* Copyright 2024-2024 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.grpc.sample;

import static org.assertj.core.api.Assertions.assertThat;

import io.grpc.ManagedChannel;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.context.ConfigurableApplicationContext;

@SpringBootTest
class InProcessApplicationContextInitializerTests {
Copy link

Choose a reason for hiding this comment

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

should this test class be in spring-grpc-test module instead?

Copy link
Member

Choose a reason for hiding this comment

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

It depends if the test jar depends on Spring Boot or not. I'm happy for it to be here for now anyway -it could also be a nested class inside the existing GrpcServerIntegrationTests (which I see below you already have, so maybe we don't need this one at all?).


@Nested
@SpringBootTest(properties = { "spring.grpc.server.host=0.0.0.0", "spring.grpc.server.port=0",
"spring.grpc.inprocess.enabled=true" })
class WithInProcessEnabled {

@Autowired
private ConfigurableApplicationContext context;

@Test
void testInitialize_WithEnabledProperty_ShouldRegisterServerAndChannel() {
ManagedChannel channel = context.getBean("grpcInProcessChannel", ManagedChannel.class);
assertThat(channel).isNotNull();
assertThat(channel.isShutdown()).isFalse();
}

}

@Nested
@SpringBootTest(properties = { "spring.grpc.server.host=0.0.0.0", "spring.grpc.server.port=0",
"spring.grpc.inprocess.enabled=false" })
class WithInProcessDisabled {

@Autowired
private ConfigurableApplicationContext context;

@Test
void testInitialize_WithDisabledProperty_ShouldNotRegisterServerAndChannel() {
assertThat(context.containsBean("grpcInProcessChannel")).isFalse();
}

}

@Nested
@SpringBootTest(properties = { "spring.grpc.server.host=0.0.0.0", "spring.grpc.server.port=0",
"spring.grpc.inprocess.enabled=true" })
Copy link
Member

Choose a reason for hiding this comment

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

This is the same test as WithInProcessEnabled? Probably we can just have one (and not set the property - just use the default).

class WithDefaultInProcessEnabled {

@Autowired
private ConfigurableApplicationContext context;

@Test
void testDefaultEnabledProperty_ShouldRegisterServerAndChannel() {
ManagedChannel channel = context.getBean("grpcInProcessChannel", ManagedChannel.class);
assertThat(channel).isNotNull();
}

}

}
4 changes: 4 additions & 0 deletions spring-grpc-test/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,9 @@
<groupId>io.grpc</groupId>
<artifactId>grpc-testing</artifactId>
</dependency>
<dependency>
<groupId>io.grpc</groupId>
<artifactId>grpc-inprocess</artifactId>
</dependency>
</dependencies>
</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/*
* Copyright 2024-2024 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.grpc.test;

import org.springframework.context.ApplicationContextInitializer;
import org.springframework.context.ConfigurableApplicationContext;

import io.grpc.ManagedChannel;
import io.grpc.Server;
import io.grpc.inprocess.InProcessChannelBuilder;
import io.grpc.inprocess.InProcessServerBuilder;

/**
* An {@link ApplicationContextInitializer} that configures and registers an in-process
* gRPC {@link Server} and {@link ManagedChannel} within a Spring
* {@link ConfigurableApplicationContext}. This initializer is intended for testing
* environments, allowing gRPC communication within the same process without network
* overhead.
*
* @author Andrei Lisa
*/
public class InProcessApplicationContextInitializer
Copy link
Member

Choose a reason for hiding this comment

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

Is there a special reason this is an ApplicationContextInitializer? What is the expected usage? I was sort of hoping that a user would be able to easily write an integration test that uses the in-process channel instead of the tcp one. Doesn't that mean we should make it look as much like the existing GrpcServerLifecycle pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe ApplicationContextInitializer is a better fit here, as it allows us to set up a simple in-process channel builder directly at application startup and ensures that it’s automatically shut down when the process ends. This approach keeps things lightweight and aligns well with the purpose of the in-process channel, as we don’t need the full lifecycle management that GrpcServerLifecycle provides.

Copy link
Member

Choose a reason for hiding this comment

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

I disagree, and I'm not really sure I want to merge this yet. It works up to a point, but it doesn't stop the tcp server starting, which feels odd to me (I was hoping for a drop in replacement). It's also an unusual way to implement this kind of feature, and I'm not convinced it won't create problems down the road. Looking at the test containers support was a good suggestion, so I'd like to understand what we are missing there before we do anything else here.

implements ApplicationContextInitializer<ConfigurableApplicationContext> {

private static final String PROPERTY_SOURCE_NAME = "spring.grpc.inprocess.enabled";

private static final String CHANNEL_NAME = "grpcInProcessChannel";

@Override
public void initialize(ConfigurableApplicationContext applicationContext) {
String inProcessEnabled = applicationContext.getEnvironment().getProperty(PROPERTY_SOURCE_NAME, "true");

if ("true".equalsIgnoreCase(inProcessEnabled) && isJarOnClasspath()) {
try {
String serverName = InProcessServerBuilder.generateName();

Server grpcServer = InProcessServerBuilder.forName(serverName).directExecutor().build().start();
Copy link
Member

Choose a reason for hiding this comment

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

We are starting this server without registering any services. This is a bug, and it should have had a test. Feels like maybe a sign that we would be better sticking more closely to the GrpcServerFactory pattern. I'm still open to not doing that, but there has to be a test, and the services have to get registered, at a minimum. What about interceptors? They probably need to get registered as well (maybe with exceptions for testing).

Copy link
Contributor Author

@andreilisa andreilisa Nov 12, 2024

Choose a reason for hiding this comment

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

Yes, you are right and also I’m aware of the missing service registration and encountered the same issue in testing. I agree that this indicates a need to align more closely with the GrpcServerFactory pattern, especially for consistency in managing service and interceptor registration. At a minimum, I’ll ensure that services are properly registered and that we have a test to validate this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dsyer Can you please check the changes again ? II implemented a factory for the in-process channel that now ensures the correct registration of services, which resolves the bugs we encountered earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dsyer Hello, what do think about this PR ?


ManagedChannel grpcChannel = InProcessChannelBuilder.forName(serverName).directExecutor().build();
applicationContext.getBeanFactory().registerSingleton(CHANNEL_NAME, grpcChannel);

applicationContext.addApplicationListener(new InProcessServerShutdownListener(grpcServer, grpcChannel));

}
catch (Exception e) {
throw new RuntimeException("Failed to initialize in-process gRPC server", e);
}
}
}

private boolean isJarOnClasspath() {
Copy link

Choose a reason for hiding this comment

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

Consider re-use existing method ClassUtils.isPresent

try {
Class.forName("io.grpc.inprocess.InProcessChannelBuilder");
return true;
}
catch (ClassNotFoundException e) {
return false;
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* Copyright 2024-2024 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.grpc.test;

import org.springframework.context.ApplicationListener;
import org.springframework.context.event.ContextClosedEvent;

import io.grpc.ManagedChannel;
import io.grpc.Server;

/**
* Listener that automatically shuts down the in-process gRPC {@link Server} and
* {@link ManagedChannel} when the Spring
* {@link org.springframework.context.ApplicationContext} is closed. This helps to ensure
* proper resource cleanup after the application context is no longer active, avoiding the
* need for manual shutdown calls in tests or other classes.
*
* @author Andrei Lisa
*/
final class InProcessServerShutdownListener implements ApplicationListener<ContextClosedEvent> {

private final Server grpcServer;

private final ManagedChannel grpcChannel;

InProcessServerShutdownListener(Server grpcServer, ManagedChannel grpcChannel) {
this.grpcServer = grpcServer;
this.grpcChannel = grpcChannel;
}

@Override
public void onApplicationEvent(ContextClosedEvent event) {
if (grpcChannel != null) {
grpcChannel.shutdownNow();
}
if (grpcServer != null) {
grpcServer.shutdownNow();
}
}

}
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Application Context Initializers
org.springframework.context.ApplicationContextInitializer=\
org.springframework.grpc.test.ServerPortInfoApplicationContextInitializer
org.springframework.grpc.test.ServerPortInfoApplicationContextInitializer,\
org.springframework.grpc.test.InProcessApplicationContextInitializer
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"groups": [],
"properties": [
{
"name": "spring.grpc.inprocess.enabled",
"defaultValue": "true"
}
]
}