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

DynamicPortsBundle should allow custom port finder #545

Closed
sleberknight opened this issue Oct 15, 2024 · 1 comment · Fixed by #548
Closed

DynamicPortsBundle should allow custom port finder #545

sleberknight opened this issue Oct 15, 2024 · 1 comment · Fixed by #548
Assignees
Labels
new feature A new feature such as a new class, method, package, group of classes, etc.
Milestone

Comments

@sleberknight
Copy link
Member

With the addition of #536 , PortAssigner can accept a FreePortFinder to determine how to find free ports.

This task is to modify DynamicPortsBundle to permit assignment of a FreePortFinder, which will need to be added to DynamicPortsConfiguration.

@sleberknight sleberknight added the new feature A new feature such as a new class, method, package, group of classes, etc. label Oct 15, 2024
@sleberknight sleberknight added this to the 3.10.0 milestone Oct 15, 2024
@sleberknight sleberknight self-assigned this Oct 15, 2024
@sleberknight
Copy link
Member Author

After looking into this more, I found that the PortAssigner really doesn't need a LocalPortChecker anymore, since the FreePortFinder is now responsible for finding ports. After release 3.9.0 when the FreePortFinder was added, the only time the LocalPortChecker is used is if the FreePortFinder is not specified when building a PortAssigner. In that case, the behavior in the constructor is to create a new RandomFreePortFinder instance and pass the LocalPortChecker to its constructor. Here is the relevant part of the constructor implementation:

@Builder
private PortAssigner(LocalPortChecker localPortChecker,
                     TlsContextConfiguration tlsConfiguration,
                     PortAssignment portAssignment,
                     FreePortFinder freePortFinder,
                     @Nullable AllowablePortRange allowablePortRange,
                     ServerFactory serverFactory,
                     PortSecurity portSecurity) {

    this.localPortChecker = Optional.ofNullable(localPortChecker).orElse(new LocalPortChecker());

    // ...other assignments elided...

    this.freePortFinder = Optional.ofNullable(freePortFinder)
            .orElseGet(() -> new RandomFreePortFinder(this.localPortChecker));

     // ...more assignments elided...
}

But if a FreePortFinder is supplied to the PortAssigner builder, then the localPortChecker field is never used. This was an oversight when I added FreePortFinder, since the previous behavior was to use the localPortChecker when finding ports. Once that behavior was removed from PortAssigner, the localPortChecker field really serves no purpose anymore. As a result, we should remove it from PortAssigner. Note this will be a breaking API change and thus cause a major version bump to 4.0.0.

sleberknight added a commit that referenced this issue Oct 17, 2024
* Configure a FreePortFinder when creating the PortAssigner
  inside DynamicPortsBundle.
* Remove LocalPortChecker from DynamicPortsConfiguration.
* Add FreePortFinder to DynamicPortsConfiguration.
* Remove LocalPortChecker from PortAssigner.
* Make FreePortFinder support Jackson's polymorphic deserialization
  by adding JsonTypeInfo to FreePortFinder, adding JsonTypeName
  annotation to the three FreePortFinder impls, and adding an
  io.dropwizard.jackson.Discoverable in META-INF/services, plus
  org.kiwiproject.dropwizard.util.startup.FreePortFinder also
  in META-INF/services.
* Lots of test refactoring and new tests.
* Misc: fix Javadoc in StartupLockConfiguration

Closes #545
Closes #546
Closes #547
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature A new feature such as a new class, method, package, group of classes, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant