[PATCH v1] Fix errors generated when multiple IRC clients talk to a single IRC proxy.

[PATCH v1] Fix errors generated when multiple IRC clients talk to a single IRC proxy.

[David Edmondson]

If multiple IRC clients are connected to a single IRC proxy, an
instance of erc can receive the response to a NAMES request issued by
another client. Given that this instance of erc didn't initiate the
NAMES request, `erc-channel-begin-receiving-names' will not have been
called and `erc-channel-new-member-names' will be nil.

To avoid this causing problems, initialise
`erc-channel-new-member-names' by calling
`erc-channel-begin-receiving-names' if it is nil.
---

Michael Olson asked me to send this to emacs-devel after I had posted it
to erc-discuss. I'm not subscribed to the list - please include me
directly in any replies.

 erc.el | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/erc.el b/erc.el
index 6e37f36..320cc02 100644
--- a/erc.el
+++ b/erc.el
@@ -4539,11 +4539,12 @@ received.  Should be called with the current buffer set to the
 channel buffer.
 
 See also `erc-channel-begin-receiving-names'."
-  (maphash (lambda (nick user)
-	     (if (null (gethash nick erc-channel-new-member-names))
-		 (erc-remove-channel-user nick)))
-	   erc-channel-users)
-  (setq erc-channel-new-member-names nil))
+  (when erc-channel-new-member-names
+    (maphash (lambda (nick user)
+	       (if (null (gethash nick erc-channel-new-member-names))
+		   (erc-remove-channel-user nick)))
+	     erc-channel-users)
+    (setq erc-channel-new-member-names nil)))
 
 (defun erc-parse-prefix ()
   "Return an alist of valid prefix character types and their representations.
@@ -4600,6 +4601,11 @@ channel."
 		  op 'off
 		  voice 'off))
 	  (when updatep
+	    ;; If we didn't issue the NAMES request (consider two clients
+	    ;; talking to an IRC proxy), `erc-channel-begin-receiving-names'
+	    ;; will not have been called, so we have to do it here.
+	    (unless erc-channel-new-member-names
+	      (erc-channel-begin-receiving-names))
 	    (puthash (erc-downcase name) t
 		     erc-channel-new-member-names)
 	    (erc-update-current-channel-member
-- 
2.1.0

Re: [PATCH v1] Fix errors generated when multiple IRC clients talk to a single IRC proxy.

[Thien-Thi Nguyen]

[-- Attachment #1: Type: text/plain, Size: 1344 bytes --]

() David Edmondson <dme@dme.org>
() Wed, 8 Oct 2014 15:03:17 +0100

   +  (when erc-channel-new-member-names
   +    (maphash (lambda (nick user)
   +	       (if (null (gethash nick erc-channel-new-member-names))
   +		   (erc-remove-channel-user nick)))
   +	     erc-channel-users)
   +    (setq erc-channel-new-member-names nil)))

I don't know if this is the case for Emacs Lisp, but i've been
burned when (and am thus now leery of) doing a destructive
operation on the hash table while hash walking, elsewhere
(here, ‘erc-remove-channel-user’ boils down to ‘remhash’).

Better to restructure as two passes: accumulation + destruction.
That way, we cleanly avoid a bug, whether manifest or latent.

(Someone will now point out how the hash table impl avoids this
problem entirely.  Well, good!  That just shifts the "bug" to
one of documenting this guarantee, which is easier to fix short
term, but perhaps harder to live w/ long term (e.g., when Emacs
is rebased on Guile).  "But ttn your words are vague, are you
trying to be obtuse?"  Yes, and both right and wrong, too!  :-D)

-- 
Thien-Thi Nguyen
   GPG key: 4C807502
   (if you're human and you know it)
      read my lisp: (responsep (questions 'technical)
                               (not (via 'mailing-list)))
                     => nil

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH v1] Fix errors generated when multiple IRC clients talk to a single IRC proxy.

[Thien-Thi Nguyen]

[-- Attachment #1: Type: text/plain, Size: 371 bytes --]

() Thien-Thi Nguyen <ttn@gnu.org>
() Wed, 15 Oct 2014 08:58:55 +0200

   right and wrong

Drat, caffeine underflow: s/wrong/acute/.  :-/

-- 
Thien-Thi Nguyen
   GPG key: 4C807502
   (if you're human and you know it)
      read my lisp: (responsep (questions 'technical)
                               (not (via 'mailing-list)))
                     => nil

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH v1] Fix errors generated when multiple IRC clients talk to a single IRC proxy.

[David Edmondson]

On Wed, Oct 15 2014, Thien-Thi Nguyen wrote:
> () David Edmondson <dme@dme.org>
> () Wed, 8 Oct 2014 15:03:17 +0100
>
>    +  (when erc-channel-new-member-names
>    +    (maphash (lambda (nick user)
>    +	       (if (null (gethash nick erc-channel-new-member-names))
>    +		   (erc-remove-channel-user nick)))
>    +	     erc-channel-users)
>    +    (setq erc-channel-new-member-names nil)))
>
> I don't know if this is the case for Emacs Lisp, but i've been
> burned when (and am thus now leery of) doing a destructive
> operation on the hash table while hash walking, elsewhere
> (here, ‘erc-remove-channel-user’ boils down to ‘remhash’).
>
> Better to restructure as two passes: accumulation + destruction.
> That way, we cleanly avoid a bug, whether manifest or latent.

I understand your concern, however this is not that bug.

Re: [PATCH v1] Fix errors generated when multiple IRC clients talk to a single IRC proxy.

[Stefan Monnier]

> I don't know if this is the case for Emacs Lisp, but i've been
> burned when (and am thus now leery of) doing a destructive
> operation on the hash table while hash walking, elsewhere
> (here, ‘erc-remove-channel-user’ boils down to ‘remhash’).

It's indeed a dangerous area.  We intend to guarantee the following properties:
- if an element is in the table during the whole duration of maphash,
  then maphash will see it.
- if an element is not in the table during any part of the duration of maphash,
  then maphash will not see it.

So if an element is removed, maphash may still see it.  And if it gets
added, maphash may miss it.  And if it gets changed, maphash may see the
old value.


        Stefan

RE: [PATCH v1] Fix errors generated when multiple IRC clients talk to a single IRC proxy.

[Mirek Kaim]

i didn't take a look at erc source yet, but if that is the case, then it's a proof of terribly, and i mean TERRIBLY, bad design. irc 
protocol by its very definition is asynchronous and whatever gets 
received, should be processed regardless of the sender of the command 
that caused the response to be received.

i'm using erc and i'm moderately happy with it, but knowing how it was written, i guess when the time comes that i'll have some spare time, instead of writing patches for it, i'll just code my own irc client for emacs. how can an irc client be working like this is beyond me, it's like it was written without reading rfcs at all.


unic0rn


----------------------------------------
> If multiple IRC clients are connected to a single IRC proxy, an
> instance of erc can receive the response to a NAMES request issued by
> another client. Given that this instance of erc didn't initiate the
> NAMES request, `erc-channel-begin-receiving-names' will not have been
> called and `erc-channel-new-member-names' will be nil.