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

Handle lost connections and EOF during readlines #73

Merged
merged 4 commits into from
Apr 27, 2017
Merged

Handle lost connections and EOF during readlines #73

merged 4 commits into from
Apr 27, 2017

Conversation

warsaw
Copy link
Contributor

@warsaw warsaw commented Apr 26, 2017

Test cases for connection problems during readline()

Also, add some much improved debugging options for when the test suite is run
with the -E option.

Update the documentation.

Closes #61

warsaw added 3 commits April 26, 2017 11:40
Also, add some much improved debugging options for when the test suite is run
with the -E option.

Update the documentation.
Lift some additional (non-line-length) tests from #59.
@warsaw
Copy link
Contributor Author

warsaw commented Apr 26, 2017

I grabbed the few additional non-line-length related tests from #59 although I'm not sure they don't cover cases already covered elsewhere. I just wanted to prove to myself that this PR handles those cases too.

I feel pretty good about this one, even if we're not technically handling EOF correctly. It does require checking for self._connection_closed in SMTP.eof_received() which is a bit ugly, but I'd like to land this branch as is, and then do a follow on branch if necessary to address that, and possibly switch over to closing the transport instead of calling connection_lost(). But at least now I think we have some good test cases.

What do you think @kozzztik and @Mortal ?

Copy link
Contributor

@Mortal Mortal left a comment

Choose a reason for hiding this comment

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

Looks good to me.

client.docmd('DATA')
# Start sending the DATA but send an EOF before that completes,
# i.e. before the .\r\n
client.send(b'From: <anne@example.com>\0x4')
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be \x04.

client.helo('example.com')
# Start sending a command but reset the connection before that
# completes, i.e. before the \r\n
client.send('MAIL FROM: <anne')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be a bytestring for consistency with the other tests. (Does SMTP.send even accept a str?)

@Mortal
Copy link
Contributor

Mortal commented Apr 26, 2017

Does sending \x04 on the wire really cause an EOF? I thought signalling EOF would be simply calling client.close()?

@warsaw
Copy link
Contributor Author

warsaw commented Apr 26, 2017

@Mortal I think you're right that the \x04 isn't really necessary. That looks like a hanger-on from a previous local commit. Good catch.

@warsaw
Copy link
Contributor Author

warsaw commented Apr 26, 2017

Oh, also, yes SMTP.send() does accept a string in the command_encoding which defaults to ascii.

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