Library review

[millerlogic]

Hi, I am reviewing the various IRC libraries deciding which to use, and below are the issues I found with this one. I will likely not be using this library nor fixing the issues, but thought this info would be useful.

• Race condition and duplicated data with AcknowledgedCaps: after disconnection, Reconnect is called, which calls Connect, which calls negotiateCaps again, adding the same callback multiple times, causing races with itself.
• nickcurrent not always updated in lock.
• AddCallback after RemoveCallback can overwrite a callback due to id reuse.
• RunCallbacks: event.Ctx doesn't cancel the WithTimeout.
• Incoming CTCP Replies not handled separately from NOTICE, even though CTCP is handled separately from PRIVMSG. I suppose it's too late to enable this by default without bumping the major version number.
• Encoding handling: it's usually not sufficient to just change the encoding, the proper handling is to set a default (which is usually either latin-1 or latin-9) but first attempt UTF-8, fallback to the default encoding if UTF-8 validation has any errors, on a per message basis. I guess this is less of an issue than a critique of how encoding is currently (not well) handled by the lib in its attempt.
• Document how event callbacks are called with respect to goroutines and processing new commands from the server.
• Document how it is safe to read public fields of the Connection. e.g. ErrorChan directly reads public field Error which should not be called during a Connect; and AcknowledgedCaps might be safe to use after a particular command from the server, or start using mutexes for these things.

[slingamn]

I addressed most of this in my fork: https://github.com/goshuirc/irc-go/tree/master/ircevent

1. Fixed issue with capability negotiation on reconnection
2. Fixed synchronization of nickcurrent, AcknowledgedCaps, etc.
3. Fixed callback ID reuse
4. Fixed the context cancellation issue by removing support for callback timeouts and concurrent execution of callbacks (my thinking was that it's not safe for callbacks to take macroscopic time anyway --- slow handlers should be moved to separate goroutines, either with go or with a queue system)
5. CTCP support: not fixed (I disabled CTCP support by default; however, I would consider making the change described above)
6. Encoding support was removed (the new API exposes raw bytes only)

Some additional changes:

1. Support for parsing 005 RPL_ISUPPORT
2. Support for IRCv3 CAP version 302, including parsing capability values
3. Support for IRCv3 batch and labeled-response
4. Fixed the deadlock from deadlock when writeLoop exits with an error #131
5. Fixed the injection issues from input should be sanitized to avoid CR-LF injection #127 (the message library is now very aggressive about refusing to emit invalid IRC lines; escaping/replacing is still left up to the caller though)

[IceflowRE]

Maybe do some PRs regarding some issues to this library?

[slingamn]

Apart from the issues that were covered by #132, I don't think I can. I rewrote parts of the library line-by-line, changed the API, integrated the other irc-go libraries (in particular the message parser/assembler) and in general made incompatible changes.

I understand that this might be a breach of etiquette, but for most use cases I would recommend that people simply switch to using the fork. The issues with the original codebase are fairly deep and it's challenging to fix them "in place" with a series of scope-limited patches.