-
Notifications
You must be signed in to change notification settings - Fork 423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
This fixes the IPv6-Issue #90 for me. #91
Conversation
…breaks anything, though. My hubot still works fine.
Nope it's not stable. For some reason, my hubot just crashed with this stacktrace. :-(
|
Do you know of a public IRC server I can connect to to try this out ? |
Better still, since I don't easily have any client capable of ipv6 at the moment, if you could collect some raw data and paste it here, that'd be really handy :-) If you put something like: console.log('parseMessage(', line, ')'); At the start of the parseMessage function, and then just pasted some of the raw output from whois queries, I think I could poke around and see what needs fixing. Thanks, |
With all the experimentation I've done, it seems that your patch works fine. Are you sure the hubot stacktrace you got wasn't related to some other issue? |
I'm collecting a few instances. So far, it has crashed on me twice. I haven't yet looked into it (so far it works fair enough for a proof-of-concept). i'll post the incoming lines as soon as i can get to it. Thanks for the support so far! |
No problems. I might hold off merging your pull request until you've collected a bit more information. Let me know how things go. |
Here's the output of one whois-attempt. I just replaced the actual domain with "domain"
i'll keep the console.log in place for the next few days. If it continues to crash, i'll let you know. |
well... i started recording all my network traffic and wrote extensive logfiles. But over the past week the node-irc hasn't crashed once! It was possibly just some random coincidence. I'll keep the logs/recordings in place for a few weeks (just in case). |
Awesome. Just as i was writing the last comment, i realize that hubot is no longer in the channel :-/ Stacktrace:
Last parseMessages:
I can now safely reproduce the bug by just typing /part into my IRC-Client. I also tested the same thing before my patch and it does not crash without my modification. In essence this means: Hold off merging a little longer. I'll look into it. |
Okay, i traced it back to https://github.com/martynsmith/node-irc/blob/v0.3.3/lib/irc.js#L329 - here the chanData() call will return undefined, which is why the subsequent delete will fail. |
aaaah: the this.chans object in https://github.com/martynsmith/node-irc/blob/v0.3.3/lib/irc.js#L441 looks like this in my test:
note the leading colon of the key and servername fields. I guess my patch is really not ready for primetime, yet. |
The error appears to be when the bot is joining. The line in question:
parses to
|
(thinking aloud ahead, you may skip over large parts of this :-D) In https://github.com/martynsmith/node-irc/blob/v0.3.3/lib/irc.js#L735 the rest of the aforementioned line will be
by then, the so-far-parsed message will be
It might be fixed by changing the replace statement in https://github.com/martynsmith/node-irc/blob/v0.3.3/lib/irc.js#L724 to insert one singular whitespace instead of empty string:
But then the channel won't get created in the first place (i.e. it never gets to line https://github.com/martynsmith/node-irc/blob/v0.3.3/lib/irc.js#L305 ), because the self.nick was not set (it is an empty string in my case. It is correctly set in the connect-listener, but then overwritten in the listener for 'raw' in case 001. The line that causes it is this:
which parses to
the leading empty string in args comes from the Long story short: i'll need to come up with a better patch. |
i came up with a solution that uses a regex instead of the indexOf. It survives the /part that killed the previous approach, but also works with all the collected lines i discussed here and in #90. |
…ng join-messages (see martynsmith#91)
I've been running this version since my last commit. Since it didn't crash or cause any problems, i think it might be worth merging. |
This fixes the IPv6-Issue #90 for me.
I am not sure if it breaks anything, though. My hubot still works fine.