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

Eager listener init #405

Merged
merged 4 commits into from
Sep 11, 2023
Merged

Eager listener init #405

merged 4 commits into from
Sep 11, 2023

Conversation

mmatczuk
Copy link
Contributor

@mmatczuk mmatczuk commented Sep 8, 2023

No description provided.

The listener injection is not used and collides with the followup changes.
@mmatczuk mmatczuk force-pushed the mmt/eager_listener_init branch from 5ac4afa to 9f348bd Compare September 8, 2023 13:55
@Choraden
Copy link
Contributor

Choraden commented Sep 8, 2023

I don't like that there is more the one subject which closes the listener i.e.Proxy.Close and Proxy.Run, but it's not wrong though, just my personal feeling.

@Choraden
Copy link
Contributor

Choraden commented Sep 8, 2023

How about documenting that one needs to close the Proxy once created?

@mmatczuk
Copy link
Contributor Author

I don't like that there is more the one subject which closes the listener i.e.Proxy.Close and Proxy.Run, but it's not wrong though, just my personal feeling.

It's needed in case of martian so that it stop listening, it is indeed wrong.

This moves network address availability  from runtime to initialization / creation.

With that it's not possible to create two servers with the same address.
This is better for error reporting.
If runctx is used we can fail fast if we know we will not be able to start listener.
@mmatczuk
Copy link
Contributor Author

Added comments.

@Choraden
Copy link
Contributor

I'm wondering if we could add Proxy::close() and call it in both Proxy:Close(), Proxy:Run() to indicate that they both do the same.

This moves network address availability  from runtime to initialization / creation.
@mmatczuk mmatczuk force-pushed the mmt/eager_listener_init branch from 9f348bd to 995b769 Compare September 11, 2023 09:08
@mmatczuk
Copy link
Contributor Author

Done.

@mmatczuk mmatczuk merged commit dcb33d4 into main Sep 11, 2023
@mmatczuk mmatczuk deleted the mmt/eager_listener_init branch September 11, 2023 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants