Skip to content

bpo-36348: IMAP4.logout() doesn't ignore exc#12411

Merged
vstinner merged 1 commit intopython:masterfrom
vstinner:imaplib_logout
Apr 15, 2019
Merged

bpo-36348: IMAP4.logout() doesn't ignore exc#12411
vstinner merged 1 commit intopython:masterfrom
vstinner:imaplib_logout

Conversation

@vstinner
Copy link
Copy Markdown
Member

@vstinner vstinner commented Mar 18, 2019 •
edited
Loading

The imap.IMAP4.logout() method no longer ignores silently arbitrary
exceptions.

Changes:

  • The IMAP4.logout() method now expects a "BYE" untagged response,
    rather than relying on _check_bye() which raises a self.abort()
    exception.
  • IMAP4.exit() now does nothing if the client already logged out.
  • Add more debug info if test_logout() tests fail.

https://bugs.python.org/issue36348

@vstinner vstinner requested review from a team and bitdancer March 18, 2019 15:14
matrixise
matrixise reviewed Mar 18, 2019
View reviewed changes
Lib/imaplib.py Outdated
Copy link
Copy Markdown
Member

@matrixise matrixise Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@matrixise
Copy link
Copy Markdown
Member

matrixise commented Mar 18, 2019

title: "does not ignore"?

@vstinner
bpo-36348: IMAP4.logout() doesn't ignore exc
661338c 
The imap.IMAP4.logout() method no longer ignores silently arbitrary
exceptions.

Changes:

* The IMAP4.logout() method now expects a "BYE" untagged response,
  rather than relying on _check_bye() which raises a self.abort()
  exception.
* IMAP4.__exit__() now does nothing if the client already logged out.
* Add more debug info if test_logout() tests fail.
@vstinner vstinner changed the title bpo-36348: IMAP4.logout() don't ignore bugs bpo-36348: IMAP4.logout() doesn't ignore exc Mar 18, 2019
@vstinner
Copy link
Copy Markdown
Member Author

vstinner commented Mar 18, 2019

I'm not sure if this change will fix the random failures on buildbot, since _command() wraps socket exceptions into self.abort(). Example if you revert my __exit__() change:

======================================================================
ERROR: test_with_statement_logout (test.test_imaplib.NewIMAPSSLTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vstinner/prog/python/master/Lib/imaplib.py", line 976, in _command
    self.send(data + CRLF)
  File "/home/vstinner/prog/python/master/Lib/imaplib.py", line 318, in send
    self.sock.sendall(data)
  File "/home/vstinner/prog/python/master/Lib/ssl.py", line 1018, in sendall
    return super().sendall(data, flags)
OSError: [Errno 9] Bad file descriptor

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/vstinner/prog/python/master/Lib/test/test_imaplib.py", line 461, in test_with_statement_logout
    self.assertIsNone(server.logged)
  File "/home/vstinner/prog/python/master/Lib/imaplib.py", line 276, in __exit__
    self.logout()
  File "/home/vstinner/prog/python/master/Lib/imaplib.py", line 628, in logout
    typ, dat = self._simple_command('LOGOUT')
  File "/home/vstinner/prog/python/master/Lib/imaplib.py", line 1200, in _simple_command
    return self._command_complete(name, self._command(name, *args))
  File "/home/vstinner/prog/python/master/Lib/imaplib.py", line 978, in _command
    raise self.abort('socket error: %s' % val)
imaplib.IMAP4.abort: socket error: [Errno 9] Bad file descriptor

@vstinner
Copy link
Copy Markdown
Member Author

vstinner commented Mar 18, 2019

I tried but failed to reproduce the buildbot failure: https://bugs.python.org/issue36348#msg338233 I ran "./python -u -m test -u all -m test_logout -v test_imaplib" 4 times in parallel and I modified the tests to remove transient_internet().

@vstinner
Copy link
Copy Markdown
Member Author

vstinner commented Mar 19, 2019

I plan to merge this PR at the end of the week.

@bitdancer, @warsaw: Would you mind to have a look?

@vstinner vstinner merged commit 74125a6 into python:master Apr 15, 2019
@vstinner vstinner deleted the imaplib_logout branch April 15, 2019 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matrixise matrixise matrixise left review comments

@bitdancer bitdancer Awaiting requested review from bitdancer

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vstinner @matrixise @the-knights-who-say-ni @bedevere-bot