Changes to state to handle auto-joining channels that have +R. #240

trevarj · 2020-08-19T12:14:38Z

If the server sends back a 477 reply, meaning that we can't join the
channel without being registered and identified with NickServ, then we
check to see if nickserv_ident is configured. If it is configured then
we wait for 10 seconds and retry joining the channel. We do this 3 times
and then stop.

Also cleaned up channel tuple and made it into a struct so that I could add a retry count for each channel.
Closes #236

libtiny_client/src/state.rs

osa1 · 2020-08-21T06:04:42Z

Thanks @trevarj ! Just added some inline comments.

libtiny_client/src/state.rs

libtiny_client/src/lib.rs

tiny/src/conn.rs

libtiny_client/src/state.rs

libtiny_client/src/lib.rs

libtiny_client/src/state.rs

trevarj · 2020-08-24T06:26:08Z

libtiny_client/src/state.rs

+    /// Channel joined state
+    join_state: JoinState,
+    /// Join attempts
+    join_attempts: u8,


Moved join_attempts out of the Joining variant in JoinState because when the state is Initial there should be join attempts. I thought it's nicer than keeping the value in Initial and in Joining.

libtiny_client/src/state.rs

trevarj · 2020-08-24T06:29:53Z

All simple feedback has been applied. I commented on some code that needs another closer review. Sorry this is taking so long!

osa1

Reading this code again, I think the initial state is fine. We can remove it, but if we do it we'll need an additional state for "currently joined channels" that we'll join again on reconnect, and move the chans vec to that field in StateInner::reset. Perhaps currently solution is fine.

I'll do another pass later.

Currnetly the state transitions are:

Initial -> Joining: when we get 477 for the channel
Initial -> Joined: when we were able to join on first attempt
Joining -> Joined: when we get a JOIN message for the channel
Joining -> Initial: I think when user does /connect while we're trying to join?
Joined -> Initial: Connection reset
Joined -> Joining: Is this currently invalid?

I think it'd be good to make sure the last one won't happen, or if it will then explain when.

Would be good to document this in the code also (maybe in the documentation for JoinState).

Btw, one bug I just discovered is if I do C-c enter and close tiny while there's a join task we want until the next iteration (~10 seconds) before tiny finally quits.

libtiny_client/src/state.rs

trevarj · 2020-08-25T10:06:31Z

* Joining -> Initial: I think when user does `/connect` while we're trying to join?

Right, just a reconnect.

Joined -> Joining: Is this currently invalid?

Yes, this is unexpected/invalid.

I will add these state transitions to the doc comment of JoinState.

I think it'd be good to make sure the last one won't happen, or if it will then explain when.

This match should prevent the bad state. I suppose it doesn't prevent the programmer from doing it somewhere later on, but hopefully the state transition doc will prevent that.

match &mut chan.join_state {
                JoinState::NotJoined => chan.set_joining(snd_abort),
                JoinState::Joining { stop_task, .. } => *stop_task = snd_abort,
                JoinState::Joined => {
                error!("Unexpected JoinState for channel.");
                      return;
                }
}

libtiny_client/src/lib.rs

osa1

Just did another pass.. This is looking quite good now. Thanks again, @trevarj !

libtiny_client/src/state.rs

osa1 · 2020-08-28T03:47:16Z

libtiny_client/src/state.rs

+                            warn!("Could not find channel in server state channel list.");
+                        }
+                    } else {
+                        warn!("Received 477 reply but nickserv_ident is not configured.");


I think this is fine, let's make this debug! maybe. Note that 477 does not always mean we need to register. Even if it does, it's perfectly fine for a user to try to join to a +R channel without being registered. It's not a problem with tiny that may need debugging later.

libtiny_client/src/state.rs

If the server sends back a 477 reply, meaning that we can't join the channel without being registered and identified with NickServ, then we check to see if nickserv_ident is configured. If it is configured then we wait for 10 seconds and retry joining the channel. We do this 3 times and then stop. Removed some unwraps. Changed UX to alert user of retries Added new Event to libtiny_client to send a channel message when a 477 reply comes from the server and we want to tell the user that a retry will be made. Also, the 477 reply from the server will show up in a channel tab which lets the user know what is going on instead of not opening a tab at all. Closing a tab that is in the process of joining a channel kills join task. Only send PART message to channel when we are actually joined (in the channel). PR feedback changes. Removed optional Sender in JoinState. Cleaned up log messages. Moved async lambda into own function. Added doc comment to JoinState. Renamed Initial state to NotJoined Fixed bug that delays quitting when join task is running More PR feedback

osa1 · 2020-08-28T07:58:34Z

libtiny_client/src/state.rs

+                        match utils::find_idx(&self.chans, |c| &c.name == chan) {
                            None => {
-                                self.chans.push((chan.to_owned(), HashSet::new()));
+                                self.chans.push(Chan::new(chan.to_owned()));


This initializes the Chan state with NotJoined instead of Joined, which is a bug probably.

I didn't understand this part. I'm assuming that when it's not found / None then the channel is not actually joined. Possibly an unexpected state that should be logged?

I can think of at least two cases where this can happen:

User sends a JOIN ... message in the server tab, instead of using /join.

The server for whatever reason adds the user to a channel. I think this happened to me before, though I don't remember the details.

I think both branches of this match should initialize the Chan state as Joined.

trevarj · 2020-08-28T08:43:03Z

libtiny_wire/src/lib.rs

-            } else {
-                false
-            };
+            let is_notice = matches!(msg_ty, MsgType::Cmd("NOTICE"));


Fixed this random clippy warning

trevarj commented Aug 19, 2020

View reviewed changes

libtiny_client/src/state.rs Show resolved Hide resolved

trevarj commented Aug 19, 2020

View reviewed changes

libtiny_client/src/state.rs Outdated Show resolved Hide resolved

osa1 reviewed Aug 21, 2020

View reviewed changes

libtiny_client/src/state.rs Show resolved Hide resolved

osa1 reviewed Aug 21, 2020

View reviewed changes

libtiny_client/src/state.rs Outdated Show resolved Hide resolved

osa1 reviewed Aug 21, 2020

View reviewed changes

libtiny_client/src/state.rs Show resolved Hide resolved

osa1 reviewed Aug 21, 2020

View reviewed changes

libtiny_client/src/state.rs Outdated Show resolved Hide resolved

osa1 reviewed Aug 21, 2020

View reviewed changes

libtiny_client/src/state.rs Outdated Show resolved Hide resolved

osa1 reviewed Aug 21, 2020

View reviewed changes

libtiny_client/src/state.rs Outdated Show resolved Hide resolved

trevarj force-pushed the nickserv-477 branch 2 times, most recently from a2d54d2 to d6b68df Compare August 21, 2020 13:49

osa1 added this to the 0.7.0 milestone Aug 22, 2020

trevarj commented Aug 22, 2020

View reviewed changes

libtiny_client/src/state.rs Outdated Show resolved Hide resolved

osa1 reviewed Aug 22, 2020

View reviewed changes

trevarj commented Aug 24, 2020

View reviewed changes

libtiny_client/src/state.rs Show resolved Hide resolved

trevarj commented Aug 24, 2020

View reviewed changes

libtiny_client/src/state.rs Show resolved Hide resolved

trevarj commented Aug 24, 2020

View reviewed changes

libtiny_client/src/state.rs Outdated Show resolved Hide resolved

osa1 reviewed Aug 25, 2020

View reviewed changes

libtiny_client/src/state.rs Outdated Show resolved Hide resolved

libtiny_client/src/state.rs Outdated Show resolved Hide resolved

trevarj commented Aug 25, 2020

View reviewed changes

libtiny_client/src/lib.rs Show resolved Hide resolved

osa1 approved these changes Aug 28, 2020

View reviewed changes

trevarj added 2 commits August 28, 2020 08:03

Added tokio macros to cargo.toml

323c630

trevarj force-pushed the nickserv-477 branch from 6e0940b to 323c630 Compare August 28, 2020 05:23

Remove tokio-macros dep

2b73253

osa1 reviewed Aug 28, 2020

View reviewed changes

Fixed initial state issue. Fixed random clippy

b1e47d7

trevarj commented Aug 28, 2020

View reviewed changes

osa1 merged commit 19f1fa2 into osa1:master Aug 28, 2020

trevarj deleted the nickserv-477 branch August 28, 2020 09:10

Changes to state to handle auto-joining channels that have +R. #240

Changes to state to handle auto-joining channels that have +R. #240

Uh oh!

Conversation

trevarj commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

osa1 commented Aug 21, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trevarj Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trevarj commented Aug 24, 2020

Uh oh!

osa1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

trevarj commented Aug 25, 2020

Uh oh!

Uh oh!

osa1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

osa1 Aug 28, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

osa1 Aug 28, 2020

Choose a reason for hiding this comment

Uh oh!

trevarj Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

osa1 Aug 28, 2020

Choose a reason for hiding this comment

Uh oh!

trevarj Aug 28, 2020

Choose a reason for hiding this comment

Uh oh!

trevarj Aug 28, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

trevarj commented Aug 19, 2020 •

edited

Loading

trevarj Aug 28, 2020 •

edited

Loading