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

Retry when Spring tests fails to start a Server with a random port obtained in advance #4395

Merged
merged 4 commits into from Sep 8, 2022

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Aug 17, 2022

Motivation:

If a random port (0) for internal services or managed services is
specified in Spring configuration, the Spring integration module
allocate available ports early and use them to configure additional
service later.


This is not a problem in production mode.
However, in testing situations, many test servers are started simultaneously
with a random port and the available ports can be acquired by them. #4391 #4329

Modifications:

  • Add RetryableArmeriaServerGracefulShutdownLifecycle to retry a
    server with a backoff at most N times.

Result:

…obtained in advance

Motivation:

If a random port (0) for internal services or managed services is
specified in Spring configuration, the Spring integration module
allocate available ports early and use them to configure additional
service later.
https://github.com/line/armeria/blob/948763acc7911c951e37dbd35132edd253ea3934/spring/boot2-actuator-autoconfigure/src/main/java/com/linecorp/armeria/spring/actuate/ArmeriaSpringActuatorAutoConfiguration.java#L258-L260
This is not a problem in production mode.
However, in testing situation, many test servers are started with a
random port and the available ports can be acquired by them. line#4391 line#4329

Modifications:

- Add `RetryableArmeriaServerGracefulShutdownLifecycle` to retry a
server with a backoff at most N times.

Result:

- This is not a perfect answer for the failure but it might reduce the
  number of failures.
- Fixes line#4391 line#4329
@codecov
Copy link

codecov bot commented Aug 17, 2022

Codecov Report

Merging #4395 (9c38f92) into master (de36a8b) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #4395      +/-   ##
============================================
- Coverage     73.84%   73.81%   -0.04%     
- Complexity    17702    17718      +16     
============================================
  Files          1505     1509       +4     
  Lines         66105    66198      +93     
  Branches       8309     8313       +4     
============================================
+ Hits          48815    48862      +47     
- Misses        13296    13330      +34     
- Partials       3994     4006      +12     
Impacted Files Coverage Δ
...meria/spring/AbstractArmeriaAutoConfiguration.java 71.42% <ø> (ø)
...ecorp/armeria/server/LengthBasedServiceNaming.java 75.00% <0.00%> (-16.67%) ⬇️
...armeria/internal/common/stream/SubscriberUtil.java 72.22% <0.00%> (-11.12%) ⬇️
...m/linecorp/armeria/client/DecodedHttpResponse.java 85.00% <0.00%> (-5.00%) ⬇️
...meria/client/DefaultDnsQueryLifecycleObserver.java 76.81% <0.00%> (-2.90%) ⬇️
...rmeria/common/stream/ConcatArrayStreamMessage.java 83.14% <0.00%> (-2.25%) ⬇️
.../linecorp/armeria/client/Http2ResponseDecoder.java 70.62% <0.00%> (-2.10%) ⬇️
...ecorp/armeria/server/ParameterizedPathMapping.java 91.76% <0.00%> (-1.18%) ⬇️
...eria/internal/common/AbstractKeepAliveHandler.java 81.42% <0.00%> (-1.10%) ⬇️
...rp/armeria/common/stream/DefaultStreamMessage.java 89.47% <0.00%> (-1.06%) ⬇️
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jrhee17
Copy link
Contributor

jrhee17 commented Aug 19, 2022

Question) Would it be possible to just pass 0 for the internal/management port? (The same as we do when we create a ServerExtension).
Asking because it seems like SocketUtils will be deprecated (and has been removed).

ref: spring-projects/spring-framework#28052

@minwoox
Copy link
Member

minwoox commented Aug 23, 2022

Would it be possible to just pass 0 for the internal/management port? (The same as we do when we create a ServerExtension).

That's a good idea. 👍 We could probably pass 0 and let Armeria find the available port.

@ikhoon
Copy link
Contributor Author

ikhoon commented Aug 23, 2022

Asking because it seems like SocketUtils will be deprecated (and has been removed).

We can easily migrate SocketUtils.findAvailableTcpPort() with our own code like we did in

private static int[] unusedPorts(int numPorts) {
final int[] ports = new int[numPorts];
final Random random = ThreadLocalRandom.current();
for (int i = 0; i < numPorts; i++) {
for (;;) {
final int candidatePort = random.nextInt(64512) + 1024;
try (ServerSocket ss = new ServerSocket()) {
ss.bind(new InetSocketAddress("127.0.0.1", candidatePort));
ports[i] = candidatePort;
break;
} catch (IOException e) {
// Port in use or unable to bind.
continue;
}
}
}
return ports;
}

Would it be possible to just pass 0 for the internal/management port?

But it is a different issue. Internal services use port-based virtual hosting that needs a pre-known port number.

public VirtualHostBuilder virtualHost(int port) {
checkArgument(port >= 1 && port <= 65535, "port: %s (expected: 1-65535)", port);

So I'd like to leave it as a known issue.

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

…spring/RetryableArmeriaServerGracefulShutdownLifecycle.java

Co-authored-by: minux <songmw725@gmail.com>
@ikhoon ikhoon merged commit 8e96b96 into line:master Sep 8, 2022
@ikhoon ikhoon deleted the spring-retry-start branch September 8, 2022 03:12
ikhoon added a commit that referenced this pull request Sep 8, 2022
…om port obtained in advance (#4395)"

This reverts commit 8e96b96.
This causes build failure after Spring Boot is upgrade to 2.7.3 from 2.7.2.
@ikhoon
Copy link
Contributor Author

ikhoon commented Sep 8, 2022

This PR has been reverted because the changes cause build failures with Spring Boot 2.7.3
https://github.com/line/armeria/runs/8243288221?check_suite_focus=true

@ikhoon ikhoon restored the spring-retry-start branch September 15, 2022 09:16
heowc pushed a commit to heowc/armeria that referenced this pull request Sep 24, 2022
…obtained in advance (line#4395)

Motivation:

If a random port (0) for internal services or managed services is
specified in Spring configuration, the Spring integration module
allocate available ports early and use them to configure additional
service later.
https://github.com/line/armeria/blob/948763acc7911c951e37dbd35132edd253ea3934/spring/boot2-actuator-autoconfigure/src/main/java/com/linecorp/armeria/spring/actuate/ArmeriaSpringActuatorAutoConfiguration.java#L258-L260
This is not a problem in production mode.
However, in testing situations, many test servers are started simultaneously 
with a random port and the available ports can be acquired by them. line#4391 line#4329

Modifications:

- Add `RetryableArmeriaServerGracefulShutdownLifecycle` to retry a
server with a backoff at most N times.

Result:

- This is not a perfect answer for the failure but it might reduce the
  number of failures.
- Fixes line#4391 line#4329
heowc pushed a commit to heowc/armeria that referenced this pull request Sep 24, 2022
…om port obtained in advance (line#4395)"

This reverts commit 8e96b96.
This causes build failure after Spring Boot is upgrade to 2.7.3 from 2.7.2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failure: com.linecorp.armeria.spring.actuate.ArmeriaSpringActuatorAutoConfigurationSecureTest.normal()
3 participants