zD12 / pircbotx

Automatically exported from code.google.com/p/pircbotx
0 stars 0 forks source link

CAP System needs Rewrite #107

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
The more that I look over the CAP system, the more that I think we can do a lot 
better. Since its quite early in the morning and I don't want to rewrite things 
without getting input, I'll ask here

To me it looks like CAP support was tacked on instead of natively supported. 
Requiring the user to send CAP END doesn't seem like the best option. Requiring 
the user to write their own Listener to handle SASL authentication seems 
redundant since most users (I assume) are going to have the same listener

The SASL page I was looking at ( http://ircv3.atheme.org/extensions/sasl-3.1 ) 
shows that we don't need to send CAP LS, we can just send CAP REQ :sasl . This 
means we can handle authentication with native setters ( setSaslUsername() and 
setSaslPassword()?) in the connect() method. If a user wants to use other CAP 
methods I'm pretty sure they can just send CAP LS in onSocketConnect. This way 
it keeps our code separate from theirs

I was also considering pushing back these changes to 1.9 since I really want to 
get 1.8 out the door. Especially since CAP is such a complex feature that I 
would like to do more testing on

Thoughts?

Original issue reported on code.google.com by Lord.Qua...@gmail.com on 7 Jan 2013 at 11:28

GoogleCodeExporter commented 9 years ago
Actually, I do agree.  

Ever since the issue where the guy's bot was timing out, I've been trying to 
come up with a better model, but haven't had the time to mess around with code. 
(We are going to be renovating our house these next two weeks, so I am super 
busy)

In the current implimentation, I call CAP LS because not all servers support 
SASL (ie test installations), and this implementation handles that case safely. 
(Youd get a CAP NAK :sasl line otherwise, or maybe nothing at all, if the 
server doesnt support CAP)

In terms of hooking onSocketConnect, I doubt servers will allow multiple CAP 
sessions, so IMHO the bot should handle opening and closing the session 
-somehow-. I just cant think of how yet. The (flawed) idea behind the current 
implimentation is for listeners to do their own thing when when the server 
sends the results of CAP LS (which says what the server can do).

Just had a thought: listereners register what CAPs they want, then when CAP LS 
comes in, pass an event in. Keep count of the registered listeners, and require 
them to declare when they are done with CAP. When the counter is 0, fire CAP 
END and continue. I will flesh out this idea later fater renovating our house.

Original comment by entityreborn on 7 Jan 2013 at 4:45

GoogleCodeExporter commented 9 years ago
Would you mind if I move the CAP code then to a new 1.9 branch? 

Original comment by Lord.Qua...@gmail.com on 7 Jan 2013 at 4:47

GoogleCodeExporter commented 9 years ago
Ok, the code has been removed in the 1.8 branch. Not sure exactly how I'm going 
to use mvn release now, but that's a goal for another day

Original comment by Lord.Qua...@gmail.com on 7 Jan 2013 at 6:15

GoogleCodeExporter commented 9 years ago
Awesome.

Original comment by entityreborn on 8 Jan 2013 at 5:09

GoogleCodeExporter commented 9 years ago
Now that 1.8 is out of the door, time to tackle this issue.

Since its still required to send CAP END when finished, I think the only way I 
can think of to cleanly implement user-handlable CAP handling is to create an 
entirely new CapHandler interface since the listener system just wasn't 
designed for something like this

Flow would work something like this
 1. CAP LS
 2. We check if the server supports SASL. If so, check if the user specified a SASL username and password (and maybe authentication type?) and pass it to the server
 3. Handle any other natively supported CAP capabilities in Issue #108
 4. Now we call any added CapHandler's. If the user doesn't want to use our native CAP handling they can do anything here. 
 5. CAP END
 6. 001 Welcome to....

This seems like the most extensible and cleanest option that would work. We 
just have to figure out how were going to write the interface

Original comment by Lord.Qua...@gmail.com on 11 Jan 2013 at 8:56

GoogleCodeExporter commented 9 years ago
Looks good, but if this will be async, we need to implement some sort of 
counter and CapHandlers will need to tell the interface when they're done. If 
it's sync (which I can't see how we could do that and still keep sending and 
receiving), then it's a simple loop.

For the counter:
* Counter is at 0.
* Listener/whatever registers a caphandler, increment counter.
* Trigger all the caphandlers with the contents of the response to CAP LS.
* Cap handlers will trigger some sort of signal saying they are done with CAP. 
This decrements the counter.
* Meanwhile keep checking the counter in the main thread. Once counter is at 0 
again, trigger CAP END and continue on.

At least, this is the best I can come up with. I really don't think we can just 
do this sync/in a foreach loop, due to the need to continue sending/receiving, 
as well as some CAP events (such as SASL) don't use CAP prefixed commands (in 
SASL's example, it uses AUTHENTICATE commands).

Original comment by entityreborn on 11 Jan 2013 at 9:38

GoogleCodeExporter commented 9 years ago
I think this is finished. If you have any improvements just reopen this issue

Original comment by Lord.Qua...@gmail.com on 15 Jan 2013 at 3:51