change readpartial to readline

[dgobaud]

change readpartial to readline because readpartial can return before it reads a whole line and that messes up process_line

[TomFreudenberg]

Hi David I am not sure but I not fully agree with your pull request. At least the SMTP Protocol specifies not to have longer command lines than 1024 chars. Even that there are systems like MS Exchange which will send longer lines, they still not reach 4096 characters. So for that it is better to stay on readpartial for me. It is non blocking and will proceed valid command lines.

What do you think?

[dgobaud]

I think readpartial creates the chance for errors - see http://www.ruby-doc.org/core-2.1.2/IO.html#method-i-readpartial. Possible problems:

1. readpartial could return part of a line - it returns immediately if any data is available. Image the client has sent 1 byte of a 10 byte line. It could return that 1 byte and you would pass it to process_line even though it isn't ready to be processed.
2. I don't think readpartial stops when it finds a new line - it simply returns up to maxlen bytes so it could return more than one line and you would pass that single string to process_line which does not handle multiple lines.

[TomFreudenberg]

Your second point is not correct as I read the books. See last examples (green on black documentation) and have a look at last two readpartial lines. they exactly show that it only return until next new line.

Your first point is quite correct, your described situation could become true. I am thinking for myself weather a loop waiting for complete line is better in conjunction with readpartial or your are right with readline. Just to keep an eye on non-blocking actions. But at least also readpartial will block if no data available.

Maybe better do test before readline if there are data available other do not call (and block) with readline.

[dgobaud]

No it is the gets call in the example causing that behavior. Just tried this:

2.0.0p353 :001 > r, w = IO.pipe
=> [#<IO:fd 7>, #<IO:fd 8>]
2.0.0p353 :002 > w << "abc\ndef\n"
=> #<IO:fd 8>
2.0.0p353 :003 > r.readpartial(4096)
=> "abc\ndef\n"

[TomFreudenberg]

Hm, thanks for clarification ... so switching to readline() seems to be neccessary. What do you think about checking data availibility before calling readline()?

[dgobaud]

I don't think you need to do that.

[TomFreudenberg]

Hi David, yes, I can agree to your pull request.

[kcrawford]

Was this pull request merged. I ran into this bug today I think. I had to override process_line to process multiple lines.

[aarongough]

At the moment I am not maintaining mini-smtp-server, at least partly because I don't have any projects that would force me to validate ongoing work, and I therefore don't want to unknowingly introduce bugs into a project that I know at least worked for my previous uses...

If the content of the pull-request is vital to your work I would recommend either branching mini-smtp-server and creating a new Gem, or simply monkey-patching it in your system.

[TomFreudenberg]

Hi Kyle ( @kcrawford )

you may have a look also at our fork, where also readpartial was changed into readline as some other nice extensions.

https://github.com/4commerce-technologies-AG/midi-smtp-server/blob/master/lib/midi-smtp-server.rb#L199