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

[WIP] bpo-39259: nntplib.NNTP/NNTP_SSL now reject timeout = 0 #17936

Open
wants to merge 2 commits into
base: master
from

Conversation

@corona10
Copy link
Member

corona10 commented Jan 10, 2020

@corona10 corona10 changed the title bpo-39259: nntplib.NNTP/NNTP_SSL now reject timeout = 0 [WIP] bpo-39259: nntplib.NNTP/NNTP_SSL now reject timeout = 0 Jan 10, 2020
@corona10 corona10 force-pushed the corona10:bpo-39259-nntplib branch from e1fe47d to 6237928 Jan 10, 2020
Copy link
Member

vstinner left a comment

This refactoring is interesting. I merged your other change to reject timeout=0, you should now rebase this PR.

Currently, this PR is a refactoring and rejects timeout=0. I prefer to have one commit per change.

def _create_socket(self, timeout):
if timeout is not None and not timeout:
raise ValueError('Non-blocking socket (timeout=0) is not supported')
return socket.create_connection((self.host, self.port), timeout)

This comment has been minimized.

Copy link
@vstinner

vstinner Jan 10, 2020

Member

You should move sys.audit("nntplib.connect", self, host, port) here :-)

usenetrc, timeout)

def _create_socket(self, timeout):
self.sock = NNTP._create_socket(self, timeout)

This comment has been minimized.

Copy link
@vstinner

vstinner Jan 10, 2020

Member
Suggested change
self.sock = NNTP._create_socket(self, timeout)
self.sock = super()._create_socket(timeout)
finally:
self.sock.close()
self.ssl_context = ssl_context
NNTP.__init__(self, host, port, user, password, readermode,

This comment has been minimized.

Copy link
@vstinner

vstinner Jan 10, 2020

Member
Suggested change
NNTP.__init__(self, host, port, user, password, readermode,
super().__init__(host, port, user, password, readermode,

def _create_socket(self, timeout):
self.sock = NNTP._create_socket(self, timeout)
self.sock = _encrypt_on(self.sock, self.ssl_context, self.host)

This comment has been minimized.

Copy link
@vstinner

vstinner Jan 10, 2020

Member

try: ... except: self.sock.close() raise is needed here.

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jan 10, 2020

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.