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

Fixed withListener to make listenerSupplier lazy again #9173

Conversation

YanivKunda
Copy link

.withListener accepted a Supplier<String> that immediately got its value - defeating the purpose of using a supplier.
This change (and accompanying test) fixes it, and parses the entire listener lazily.

@YanivKunda
Copy link
Author

Resolves #9215

@YanivKunda YanivKunda force-pushed the bug/listener-supplier-is-not-used-lazily branch from 151a74c to 6f0a36f Compare September 18, 2024 06:56
@eddumelendez
Copy link
Member

H @YanivKunda, can you share what are you trying to achieve? I'm just curious if your real life code will work with the fix submitted. Your fix is technically correct but just want to see if it will fix what you want to do (I have an idea).

@YanivKunda
Copy link
Author

Actually I just encountered this code and considered it a bug without a real-life use case -
so I could only write a test to check that.
Basically I think that getting a listener that can only be computed after the container starts (similarly to the docker mapped ports) is a valid use case.
Also, if it turns out there's no real-life use case for that, meaning most users are going to pass a constant supplier, then there's no real point in keeping the method accepting a Supplier<String> - you could just have a withListener(String) method instead (or in addition, for backwards-compatibility).

@YanivKunda
Copy link
Author

@eddumelendez Do you think this is ready for review, or does it require any further work or discussion?

@kiview
Copy link
Member

kiview commented Oct 1, 2024

Thanks @YanivKunda.

Your reasoning with evaluating the supplier makes sense, but the example does not:

Basically I think that getting a listener that can only be computed after the container starts (similarly to the docker mapped ports) is a valid use case.

The configured listeners are evaluated in the configure() method, which happens before the container is created and started.

I also don't understand, why we need to make changes to a deprecated method. This issue is not present in the other withListener() methods?

@YanivKunda
Copy link
Author

@kiview you're right about my example, and I'm not sure I would be able to come up with a better one.
But it probably doesn't matter since that is deprecated - which is probably the reason @eddumelendez deprecated it in #9247.

And obviously, unless it's a critical fix, there's no need to change a deprecated method.
It's just that it wasn't deprecated when I first created this PR :-)

@YanivKunda YanivKunda closed this Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants