bpo-38615: Add timeout parameter for IMAP4 and IMAP4_SSL constructor by corona10 · Pull Request #17203 · python/cpython

corona10 · 2019-11-17T08:47:03Z

https://bugs.python.org/issue38615

ericvsmith

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

Doc/library/imaplib.rst

Doc/whatsnew/3.9.rst

Lib/imaplib.py

Misc/NEWS.d/next/Library/2019-11-17-17-32-35.bpo-38615.OVyaNX.rst

bedevere-bot · 2019-11-17T13:58:11Z

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 · 2019-11-18T05:43:37Z

@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 · 2019-11-18T05:43:40Z

Thanks for making the requested changes!

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

ericvsmith

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

Lib/test/test_imaplib.py

bedevere-bot · 2019-11-18T12:22:55Z

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 · 2019-11-19T02:57:37Z

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
bpo-38615: Add timeout parameter for IMAP4 and IMAP4_SSL constructor #17203 (comment)

bedevere-bot · 2019-11-19T02:57:40Z

Thanks for making the requested changes!

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

corona10 · 2019-11-26T14:29:18Z

@ericvsmith @python/email-team

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

corona10 · 2019-12-08T12:56:27Z

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

Doc/library/imaplib.rst

bedevere-bot · 2019-12-16T06:39:41Z

Thanks for making the requested changes!

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

Lib/imaplib.py

vstinner

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

vstinner · 2020-01-06T22:09:22Z

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

Co-Authored-By: Victor Stinner <vstinner@python.org>

corona10 · 2020-01-07T02:31:48Z

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

corona10 · 2020-01-07T02:57:21Z

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

corona10 · 2020-01-07T04:43:09Z

@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 · 2020-01-07T04:43:14Z

Thanks for making the requested changes!

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

…ythonGH-17203) imaplib.IMAP4 and imaplib.IMAP4_SSL now have an optional *timeout* parameter for their constructors. Also, the imaplib.IMAP4.open() method now has an optional *timeout* parameter with this change. The overridden methods of imaplib.IMAP4_SSL and imaplib.IMAP4_stream were applied to this change.

corona10 requested a review from a team as a code owner November 17, 2019 08:47

the-knights-who-say-ni added the CLA signed label Nov 17, 2019

bedevere-bot added the awaiting review label Nov 17, 2019

corona10 force-pushed the bpo-38615 branch from e5b9697 to b56b7aa Compare November 17, 2019 08:53

ericvsmith requested changes Nov 17, 2019

View reviewed changes

bedevere-bot removed the awaiting review label Nov 17, 2019

bedevere-bot added the awaiting changes label Nov 17, 2019

corona10 force-pushed the bpo-38615 branch from b40c7c9 to 9d1976f Compare November 18, 2019 05:37

bedevere-bot removed the awaiting changes label Nov 18, 2019

bedevere-bot added the awaiting change review label Nov 18, 2019

bedevere-bot requested a review from ericvsmith November 18, 2019 05:43

ericvsmith requested changes Nov 18, 2019

View reviewed changes

Doc/library/imaplib.rst Outdated Show resolved Hide resolved

Lib/test/test_imaplib.py Outdated Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting change review labels Nov 18, 2019

corona10 force-pushed the bpo-38615 branch from fab5f9f to 844b7ae Compare November 19, 2019 02:34

bedevere-bot added awaiting change review and removed awaiting changes labels Nov 19, 2019

bedevere-bot requested a review from ericvsmith November 19, 2019 02:57

corona10 requested a review from maxking December 8, 2019 12:55

maxking suggested changes Dec 13, 2019

View reviewed changes

Doc/library/imaplib.rst Outdated Show resolved Hide resolved

Doc/library/imaplib.rst Outdated Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting change review labels Dec 13, 2019

bedevere-bot requested a review from vstinner December 16, 2019 06:39

vstinner reviewed Jan 6, 2020

View reviewed changes

Lib/imaplib.py Outdated Show resolved Hide resolved

vstinner approved these changes Jan 6, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jan 6, 2020

corona10 and others added 11 commits January 7, 2020 11:11

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

e565171

bpo-38615: Update default timeout paramter into None

d642688

bpo-38615: Update docstring

1c6e39a

bpo-38615: Add unittest

d04e2d2

bpo-38615: Update

7725c4a

bpo-38615: update

dff8807

bpo-38615: Update imaplib.rst

7bf808a

bpo-38615: Add timeout actual functionality test

b9a2404

bpo-38615: Apply maxking's review

722271a

bpo-38615: Apply vstinner's review

3fbe5fa

Update Lib/imaplib.py

a8dee3a

Co-Authored-By: Victor Stinner <vstinner@python.org>

corona10 force-pushed the bpo-38615 branch from 9b61a44 to a8dee3a Compare January 7, 2020 02:11

bpo-38615: Fix test to be careful about server preparation time

c903dec

bedevere-bot added awaiting change review and removed awaiting merge labels Jan 7, 2020

bedevere-bot requested a review from vstinner January 7, 2020 04:43

vstinner merged commit 13a7ee8 into python:master Jan 7, 2020

bedevere-bot removed the awaiting change review label Jan 7, 2020

Uh oh!

Conversation

corona10 commented Nov 17, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericvsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Nov 17, 2019

Uh oh!

corona10 commented Nov 18, 2019

Uh oh!

bedevere-bot commented Nov 18, 2019

Uh oh!

ericvsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Nov 18, 2019

Uh oh!

corona10 commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Nov 19, 2019

Uh oh!

corona10 commented Nov 26, 2019

Uh oh!

corona10 commented Dec 8, 2019

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Dec 16, 2019

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jan 6, 2020

Uh oh!

corona10 commented Jan 7, 2020

Uh oh!

corona10 commented Jan 7, 2020

Uh oh!

corona10 commented Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Jan 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

corona10 commented Nov 17, 2019 •

edited by bedevere-bot

Loading

corona10 commented Nov 19, 2019 •

edited

Loading

corona10 commented Jan 7, 2020 •

edited

Loading