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

bpo-38615: Add timeout parameter for IMAP4 and IMAP4_SSL constructor #17203

Merged
merged 12 commits into from Jan 7, 2020

Conversation

@corona10
Copy link
Member

corona10 commented Nov 17, 2019

@corona10 corona10 requested a review from python/email-team as a code owner Nov 17, 2019
@corona10 corona10 force-pushed the corona10:bpo-38615 branch from e5b9697 to b56b7aa Nov 17, 2019
Copy link
Member

ericvsmith left a comment

Thanks for the patch. See my notes about using a default value of None. Also, tests are needed.

Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.9.rst Outdated Show resolved Hide resolved
Lib/imaplib.py Outdated Show resolved Hide resolved
Lib/imaplib.py Outdated Show resolved Hide resolved
Lib/imaplib.py Outdated Show resolved Hide resolved
Lib/imaplib.py Outdated Show resolved Hide resolved
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Nov 17, 2019

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.

@corona10 corona10 force-pushed the corona10:bpo-38615 branch from b40c7c9 to 9d1976f Nov 18, 2019
@corona10

This comment has been minimized.

Copy link
Member Author

corona10 commented Nov 18, 2019

@ericvsmith

I have made the requested changes; please review again

  • Update default timeout parameter into None
  • Add a unit test for the timeout parameter

"All arguments to commands are converted to strings," is no longer correct and needs to be fixed.

Since the PR affected constructor but the document talk about commands,
so, I don't know the history of the command arguments.
Is there any suggestion to change or what about creating an issue on bugs.python.org?

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Nov 18, 2019

Thanks for making the requested changes!

@ericvsmith: please review the changes made to this pull request.

Copy link
Member

ericvsmith left a comment

We'll need to find some way for testing the actual functionality of timing out. Maybe poplib or smtplib or something similar has some code that can be used?

Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Lib/test/test_imaplib.py Outdated Show resolved Hide resolved
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Nov 18, 2019

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.

@corona10 corona10 force-pushed the corona10:bpo-38615 branch from fab5f9f to 844b7ae Nov 19, 2019
@corona10

This comment has been minimized.

Copy link
Member Author

corona10 commented Nov 19, 2019

I have made the requested changes; please review again

  • Add timeout actual functionality test
  • Update imaplib.rst (as you commented)
  • Explain why timeout returns None instead of socket._GLOBAL_DEFAULT_TIMEOUT
    #17203 (comment)
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Nov 19, 2019

Thanks for making the requested changes!

@ericvsmith: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from ericvsmith Nov 19, 2019
@corona10

This comment has been minimized.

Copy link
Member Author

corona10 commented Nov 26, 2019

@ericvsmith @python/email-team

if you don't mind, Can I get the review for this PR?
Thank you for your precious time :)

@corona10 corona10 requested a review from maxking Dec 8, 2019
@corona10

This comment has been minimized.

Copy link
Member Author

corona10 commented Dec 8, 2019

@maxking Would you like to take a look at this PR if you don`t mind?

Doc/library/imaplib.rst Outdated Show resolved Hide resolved
Doc/library/imaplib.rst Outdated Show resolved Hide resolved
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Dec 16, 2019

Thanks for making the requested changes!

@maxking, @vstinner, @ericvsmith: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from vstinner Dec 16, 2019
Lib/imaplib.py Outdated Show resolved Hide resolved
Copy link
Member

vstinner left a comment

LGTM. Thanks for the update (reject timeout=0).

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Jan 6, 2020

Hum, 2 tests failed on the macOS :-( @corona10: Can you have a look?

======================================================================
ERROR: test_imaplib_timeout_test (test.test_imaplib.NewIMAPSSLTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/runners/2.163.1/work/cpython/cpython/Lib/test/test_imaplib.py", line 446, in test_imaplib_timeout_test
    client = self.imap_class("localhost", addr, timeout=support.LOOPBACK_TIMEOUT)
  File "/Users/runner/runners/2.163.1/work/cpython/cpython/Lib/imaplib.py", line 1307, in __init__
    IMAP4.__init__(self, host, port, timeout)
  File "/Users/runner/runners/2.163.1/work/cpython/cpython/Lib/imaplib.py", line 201, in __init__
    self.open(host, port, timeout)
  File "/Users/runner/runners/2.163.1/work/cpython/cpython/Lib/imaplib.py", line 1320, in open
    IMAP4.open(self, host, port, timeout)
  File "/Users/runner/runners/2.163.1/work/cpython/cpython/Lib/imaplib.py", line 311, in open
    self.sock = self._create_socket(timeout)
  File "/Users/runner/runners/2.163.1/work/cpython/cpython/Lib/imaplib.py", line 1311, in _create_socket
    return self.ssl_context.wrap_socket(sock,
  File "/Users/runner/runners/2.163.1/work/cpython/cpython/Lib/ssl.py", line 500, in wrap_socket
    return self.sslsocket_class._create(
  File "/Users/runner/runners/2.163.1/work/cpython/cpython/Lib/ssl.py", line 1040, in _create
    self.do_handshake()
  File "/Users/runner/runners/2.163.1/work/cpython/cpython/Lib/ssl.py", line 1309, in do_handshake
    self._sslobj.do_handshake()
socket.timeout: _ssl.c:1091: The handshake operation timed out


======================================================================
ERROR: test_imaplib_timeout_test (test.test_imaplib.NewIMAPTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/runners/2.163.1/work/cpython/cpython/Lib/test/test_imaplib.py", line 446, in test_imaplib_timeout_test
    client = self.imap_class("localhost", addr, timeout=support.LOOPBACK_TIMEOUT)
  File "/Users/runner/runners/2.163.1/work/cpython/cpython/Lib/imaplib.py", line 204, in __init__
    self._connect()
  File "/Users/runner/runners/2.163.1/work/cpython/cpython/Lib/imaplib.py", line 246, in _connect
    self.welcome = self._get_response()
  File "/Users/runner/runners/2.163.1/work/cpython/cpython/Lib/imaplib.py", line 1058, in _get_response
    resp = self._get_line()
  File "/Users/runner/runners/2.163.1/work/cpython/cpython/Lib/imaplib.py", line 1166, in _get_line
    line = self.readline()
  File "/Users/runner/runners/2.163.1/work/cpython/cpython/Lib/imaplib.py", line 322, in readline
    line = self.file.readline(_MAXLINE + 1)
  File "/Users/runner/runners/2.163.1/work/cpython/cpython/Lib/socket.py", line 704, in readinto
    return self._sock.recv_into(b)
socket.timeout: timed out
@corona10 corona10 force-pushed the corona10:bpo-38615 branch from 9b61a44 to a8dee3a Jan 7, 2020
@corona10

This comment has been minimized.

Copy link
Member Author

corona10 commented Jan 7, 2020

@vstinner This is very strange, what was happened between 3fbe5fa and a8dee3a

@corona10

This comment has been minimized.

Copy link
Member Author

corona10 commented Jan 7, 2020

I am able to reproduce in my local machine. I will take look at it.

@corona10

This comment has been minimized.

Copy link
Member Author

corona10 commented Jan 7, 2020

@vstinner I have made the requested changes; please review again

This is a potential issue.

  • When we test client, we don't wait until the server is ready.
  • Current tests are tested with timeout=None, so they wait until the server is ready.
  • So I relocation timeout test order: c903dec (None -> LOOPBACK_TIMEOUT -> 0)
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jan 7, 2020

Thanks for making the requested changes!

@vstinner, @ericvsmith, @maxking: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from vstinner Jan 7, 2020
@vstinner vstinner merged commit 13a7ee8 into python:master Jan 7, 2020
9 checks passed
9 checks passed
Docs
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20200107.8 succeeded
Details
bedevere/issue-number Issue number 38615 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.