jaraco / irc

Full-featured Python IRC library for Python.
MIT License
390 stars 85 forks source link

Line reading is incorrect, can lead to connection getting stuck forever #88

Closed ghost closed 8 years ago

ghost commented 8 years ago

The line handling is incorrect and can lead to the connection getting stuck forever:

Basically you cannot just read 16KB and wait until all of it arrives, then check for lines since in some interactivity situations you might be required to respond earlier. Therefore, the only safe way to parse IRC lines is to go byte for byte until a line break is reached, OR read a larger amount but with either a timeout or an exact known amount of bytes being present or with non-blocking sockets (neither of which the code appears to be doing),

I also removed the special buffer class from being used as well as always_iterable() because both make the code much less intuitive to read: they just introduce additional in-between abstractions that barely do anything that prevent people from understanding what the code does immediately.

ghost commented 8 years ago

After a bit of pondering, I think my fix will solve the hangs I observed with a single connection, but break multiple connections handled by a single reactor. I will think a bit about it, it probably needs to be solved with non-blocking sockets.

ghost commented 8 years ago

I made an amendment which should make it work properly with multiple connections without one blocking the others.

jaraco commented 8 years ago

I also removed the special buffer class from being used as well as always_iterable() because both make the code much less intuitive to read: they just introduce additional in-between abstractions that barely do anything that prevent people from understanding what the code does immediately.

In my opinion, always_iterable is worth learning. I do recognize that it requires the reader to know the purpose or to look up the implementation or assume from the name what it does. On the other hand, it allows the user to work from an assumption of its purpose and avoid concerns about branching and context in several lines of code. It's a capability elevating function. Arguably the author (me) should put more work into evangelizing the function or getting it incorporated into the stdlib, but for now its use should be preferred over inlining the functionality. Preferably, this type of style change should happen in an independent PR.

The line handling is incorrect and can lead to the connection getting stuck forever

I've never (or rarely) encountered this behavior, and I've run this library daily in an active environment for 6 years. Could you open a ticket and describe under what circumstances the connection gets stuck? Is it possible that simply changing the read size from 2**14 to 1, that would correct the issue? Since the issue appears to be affecting you, perhaps the read chunk size should be configurable.

ghost commented 8 years ago

I have read up on read() documentation Unix manuals again, and it appears I was wrong :-) it got stuck somewhere else, probably to quakenet simply throttling me. Sorry for making this misguided merge request

ghost commented 8 years ago

More background: I assumed read() would hang until the full amount of request bytes is reached (which it doesn't, it terminates once any data arrives), and when I "fixed" this in my fork it was just a coincidence that the irc server allowed me again..