Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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,101 @@
/*
* 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 io.grpc.ManagedChannel;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.NoSuchBeanDefinitionException;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
import org.springframework.grpc.test.InProcessApplicationContextInitializer;

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

/*
* @author Andrei Lisa
*/

@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?).


private AnnotationConfigApplicationContext context;

@BeforeEach
public void setUp() {
System.clearProperty("spring.grpc.inprocess");
context = new AnnotationConfigApplicationContext();
}

@AfterEach
public void cleanUp() {
InProcessApplicationContextInitializer.shutdown();
context.close();
}

@Nested
class WhenDefaultEnabled {

@Test
void shouldInitializeInProcessServer() {
new InProcessApplicationContextInitializer().initialize(context);
context.refresh();

ManagedChannel channel = context.getBean("grpcInProcessChannel", ManagedChannel.class);
assertThat(channel).isNotNull().isInstanceOf(ManagedChannel.class);
}

}

@Nested
class WhenDisabledByProperty {

@Test
void shouldNotInitializeInProcessServer() {
System.setProperty("spring.grpc.inprocess", "false");
new InProcessApplicationContextInitializer().initialize(context);
context.refresh();
assertThatThrownBy(() -> {
context.getBean("grpcInProcessChannel", ManagedChannel.class);
}).isInstanceOf(NoSuchBeanDefinitionException.class);
}

}

@Nested
class WhenShutdownIsCalled {

@Test
void shouldShutdownInProcessServer() {
new InProcessApplicationContextInitializer().initialize(context);
context.refresh();

ManagedChannel channel = context.getBean("grpcInProcessChannel", ManagedChannel.class);
assertThat(channel).isNotNull();

InProcessApplicationContextInitializer.shutdown();

assertThat(channel).isNotNull();
assertThat(channel.isShutdown()).isTrue();
}

}

}
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,77 @@
/*
* 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 io.grpc.ManagedChannel;
import io.grpc.Server;
import io.grpc.inprocess.InProcessChannelBuilder;
import io.grpc.inprocess.InProcessServerBuilder;
import org.springframework.context.ApplicationContextInitializer;
import org.springframework.context.ConfigurableApplicationContext;

/*
* @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";

private static final String CHANNEL_NAME = "grpcInProcessChannel";

private static Server grpcServer;

private static ManagedChannel grpcChannel;

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

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

grpcServer = InProcessServerBuilder.forName(serverName).directExecutor().build().start();

grpcChannel = InProcessChannelBuilder.forName(serverName).directExecutor().build();
applicationContext.getBeanFactory().registerSingleton(CHANNEL_NAME, 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;
}
}

public static void shutdown() {
if (grpcChannel != null)
grpcChannel.shutdownNow();
if (grpcServer != null)
grpcServer.shutdownNow();
}
Copy link

@vuhp vuhp Nov 7, 2024

Choose a reason for hiding this comment

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

I believe users should not have to call to startup or should down from their test classes.
Instead, we can leverage a post-processor to handle the shutdown process.

I think this TestcontainersLifecycleApplicationContextInitializer is a good reference

Copy link
Contributor Author

@andreilisa andreilisa Nov 9, 2024

Choose a reason for hiding this comment

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

Yes I am agree with you about this point, and also I think that an ApplicationListener on ContextClosedEvent achieves the purpose of not requiring users to manually handle the lifecycle, specifically shutdown.


}
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
Loading