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

Fix port selection bug #10

Closed
wants to merge 2 commits into from

Conversation

AlphonsG
Copy link

@AlphonsG AlphonsG commented May 9, 2022

Old conditional would not result in port 0 being selected if specified by the user.

Old conditional would not result in port 0 being selected if specified by the user.
@AlphonsG
Copy link
Author

AlphonsG commented May 9, 2022

For: #9

@Dragon2fly
Copy link
Owner

Hi @AlphonsG

Please add the test for your pull request.
You can refer to this previous test , set the port to 0 and try out.

@AlphonsG
Copy link
Author

AlphonsG commented May 13, 2022

Hello @Dragon2fly , I tried writing a test based on the one you linked, but it was failing due to the way the logging is implemented. For example, if we have port 0 assign an unused port to the logger, then if you reinitialise that logger in a spawned process (specifying port 0 again), then you won't log to the same address as the logger in the parent process. I tried passing the parent logger to the child as an argument, but nothing was logged in the child process. It seems the "fix" would need to be a bit more involved than I'd initially thought.

For anyone wanting to assign an unused port to logger_tt, I suggest they just determine the port number first and then pass that to logger_tt, for example:

with socket.socket() as s:
    s.bind(('', 0))  # bind to a free port
    port = s.getsockname()[1]

then pass the port to setup_logging.

Perhaps it can be added to the documentation then that assigning port 0 to logger_tt actually assigns port handlers.DEFAULT_TCP_LOGGING_PORT?

Thanks for requesting a test for this by the way, it would've caused issues if merged.

@Dragon2fly Dragon2fly closed this Dec 30, 2022
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