Skip to content
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

Enable auto reconnection #254

Merged
merged 3 commits into from
May 24, 2016
Merged

Enable auto reconnection #254

merged 3 commits into from
May 24, 2016

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Apr 13, 2016

🎉 irc-fw has prepared us for this moment! Closes #18.

We should be reconnecting just fine without any problems. Client already prints all the necessary messages (connecting, reconnecting, socket errors, etc). Server prevents you from sending commands while you are not connected.

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Apr 13, 2016
@maxpoulin64
Copy link
Member

This doesn't work at all due to a bug in irc-framework. It just hangs in the registration phase and eventually timeout and try again.

I don't understand that part of the irc-framework part as the code is hard to follow, but it sounds like there's some state tracking that needs to be reset that isn't.

@astorije
Copy link
Member

astorije commented May 6, 2016

Hey guys, do you think there is a fix in progress on the irc-framework side of things?

@maxpoulin64
Copy link
Member

It was fixed recently in irc-fw master, but I haven't had the occasion to retest it yet. But in theory yes, it's fixed!

@astorije
Copy link
Member

astorije commented May 8, 2016

Exciting! Can't wait to try this!

@xPaw xPaw removed the Meta: Do Not Merge This PR should not be merged. label May 12, 2016
@xPaw xPaw mentioned this pull request May 12, 2016
@xPaw xPaw added this to the ★ Next Release milestone May 13, 2016
@xPaw
Copy link
Member Author

xPaw commented May 16, 2016

This is ready to be merged, @thelounge/maintainers.

@AlMcKinlay
Copy link
Member

Well, the code looks good. I'm happy to test it if you still think it needs some testing, equally I'm happy for it to go in (Especially as we'll be releasing to a beta/rc first). So 👍 for it. Obviously the testing is quite awkward trying to get it to disconnect from something to reconnect (Especially as I use znc).

network.channels[0].pushMessage(client, new Msg({
text: "Disconnected from the network."
text: "Disconnected from the network, and will not reconnect."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any addition reason or something we can give to the user? It seems very frustrating otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous messages in log should give context (e.g. failed reconnects).

@astorije
Copy link
Member

I'll test this very soon!

@astorije astorije self-assigned this May 17, 2016
@@ -1,8 +1,24 @@
var Msg = require("./models/msg");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work and crash on start. Needs changing to ../../models/msg.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maxpoulin64
Copy link
Member

Just tested this, and it finally works! 🎉 The only thing I see that needs fixing would be the comment I just added because it crashes on start, and then I think we're finally good to ship this!

@astorije
Copy link
Member

@maxpoulin64, fixed the issued you noticed in this PR.
I'll do some tests of the overall PR tomorrow before merging!

@xPaw
Copy link
Member Author

xPaw commented May 22, 2016

Thanks @astorije, unsure how I missed that.

@astorije
Copy link
Member

It's actually fairly hard to get disconnected from Freenode on purpose! I don't know what black magic is that, but when running The Lounge locally, turning wifi, turning it on a minute or 2 later and refreshing the page, messages posted during the time off wifi appeared, even though the server was running locally. Can't explain this too well, and it might be the consequence of a few things together (IRCv3, irc-fw, some flags on Freenode, I don't know...).

However, on a network that is not that smart, I got:

screen shot 2016-05-24 at 00 06 43

The socket error is when I shut down wifi. What this shows is that when reconnecting, it tried with the same username as before, but since the nick has not quit properly, it was still hanging around and the server refused to connect properly.

Is there something that can be done here, such as trying with a different nick (old_nik+random_int)? Or is letting the reconnect fail until it succeeds good enough?

@xPaw
Copy link
Member Author

xPaw commented May 24, 2016

Remember when we argued about nick changes before?

It currently only tries to change the name if it's irc.connection.registered, unsure how we would fix it, change if it's less than 5 seconds since registration?

@astorije
Copy link
Member

Yeah, definitely not an obvious fix.
At the very least, this PR reconnects fine, and as you said in your original message, it did reconnect, so it would be transparent overnight for example.
Let's merge this and improve edge cases over time.
👍 👏 👏

@astorije astorije merged commit 7834d12 into master May 24, 2016
@astorije astorije deleted the xpaw/auto-rc branch May 24, 2016 22:43
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants