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.

Sign up for GitHub

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

Jump to bottom

Cleanup: remove max_nick_length #109

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Cleanup: remove max_nick_length #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d74cf45
Cleanup: remove max_nick_length
llmII Mar 5, 2021
ddca4a3
Merge branch 'master' into deprecate-max-nick-len
qaisjp Mar 8, 2021
0e3fc72
Merge branch 'master' into deprecate-max-nick-len
qaisjp Mar 9, 2021
2498205
Cleanup: Inform user of deprecation of config var
llmII Mar 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ The config file is a yaml formatted file with the following fields:
| `no_tls`, | Yes | false | Yes | turns off TLS |
| `cooldown_duration` | No | 86400 (24 hours) | Yes | time in seconds for a discord user to be offline before it's puppet disconnects from irc |
| `show_joinquit` | No | false | yes | displays JOIN, PART, QUIT, KICK on discord |
| `max_nick_length` | No | 30 | yes | Maximum allowed nick length |
| `ignored_irc_hostmasks` | No | | Yes | A list of IRC users identified by hostmask to not relay to Discord, uses matching syntax as in [glob](https://github.com/gobwas/glob) |
| `connection_limit` | Yes | 0 | Yes | How many connections to IRC (including our listener) to spawn (limit of 0 or less means unlimited) |

Expand Down
3 changes: 0 additions & 3 deletions bridge/bridge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,6 @@ type Config struct {
// ShowJoinQuit determines whether or not to show JOIN, QUIT, KICK messages on Discord
ShowJoinQuit bool

// Maximum Nicklength for irc server
MaxNickLength int

Debug bool
DebugPresence bool
}
Expand Down
2 changes: 1 addition & 1 deletion bridge/irc_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ func (m *IRCManager) generateNickname(discord DiscordUser) string {
suffix := m.bridge.Config.Suffix
newNick := nick + suffix

useFallback := len(newNick) > m.bridge.Config.MaxNickLength || m.bridge.ircListener.DoesUserExist(newNick)
useFallback := len(newNick) > int(m.bridge.ircListener.NickLength()) || m.bridge.ircListener.DoesUserExist(newNick)
Copy link
Owner

Choose a reason for hiding this comment

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

this acquires a lock, so might not run great when connecting 300 people. but lets leave it in here for now unless you can think of an easy better solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, might be best to handle figuring out NickLength once per OnWelcome event in the listener and set it as a field on the Listener. However, probably need to also do an on disconnect and quit ordeal so as to clear it to a "hold up, we're not connected yet) value. Probably is overkill that last part because if a server changes this between the time of a quick disconnect and reconnect there's probably going to be other issues anyway...

Same as above applies... I'll try to get this implemented like I noted above tomorrow or if you want to you can take over for it, life events are gonna keep me away from it for the rest of today :/

Copy link
Owner

Choose a reason for hiding this comment

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

That's okay, life is more important than a random internet irc bridge :) Take care of yourself!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC IRCd's send 376 (end of motd), and 422 (no motd found). Ideally the client should listen for these events and then process their prejoin commands and access ISUPPORT (which was sent earlier in 005). Should we go forward this direction instead of processing at 001?

Even if we do, I need to lock the HandleUser function out from running until 376/422 are handled and a NickLength is stored on the Listener. So to achieve a "don't lock per puppet creation" we sort of need the Listener to have a NickLength() call that with a sentinel variable that way we only lock once for the entire ordeal of getting the NickLength.

How expensive is a lock? would it actually make it impossible for 300 users to connect in the same second. I think I'm going to do the above but if you figure it's too complex or have a better idea I could go for that instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should be committed, and a separate PR to fix things to use 376/422 should be created (which would include locking). I'm not trying to split things up too much but that's possibly a change that would need far deeper review than this change here.

// log.WithFields(log.Fields{
// "length": len(newNick) > ircnick.MAXLENGTH,
// "useFallback": useFallback,
Expand Down
1 change: 0 additions & 1 deletion config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ webirc_pass: abcdef.ghijk.lmnop

show_joinquit: false # displays JOIN, PART, QUIT, KICK on discord
cooldown_duration: 86400 # optional, default 86400 (24 hours), time in seconds for a discord user to be offline before it's puppet disconnects from irc
max_nick_length: 30 # Maximum Length of a nick allowed

# You definitely should restart the bridge after changing the following:
insecure: false
Expand Down
9 changes: 4 additions & 5 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import (
"github.com/gobwas/glob"
"github.com/pkg/errors"
"github.com/qaisjp/go-discord-irc/bridge"
ircnick "github.com/qaisjp/go-discord-irc/irc/nick"
log "github.com/sirupsen/logrus"
"github.com/spf13/viper"
)
Expand Down Expand Up @@ -119,9 +118,10 @@ func main() {
//
viper.SetDefault("show_joinquit", false)
showJoinQuit := viper.GetBool("show_joinquit")
// Maximum length of user nicks aloud
viper.SetDefault("max_nick_length", ircnick.MAXLENGTH)
maxNickLength := viper.GetInt("max_nick_length")
Comment on lines -122 to -124
Copy link
Owner

Choose a reason for hiding this comment

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

lets tell the user that it's not detected at runtime and they should remove this field from their config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add it like we did for nickserv_identify (or you can if you want, I won't be able to get back to this till tomorrow, life events).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. In this one I merely inform the user, but don't actually exit the program. Let me know if we should tell the program to exit in this case.


if viper.GetInt("max_nick_length") != 0 {
log.Println("This field should be removed from your configuration - go-discord-irc now detects the proper max accepted nick length at runtime.")
}

if webIRCPass == "" {
log.Warnln("webirc_pass is empty")
Expand Down Expand Up @@ -167,7 +167,6 @@ func main() {
ChannelMappings: channelMappings,
CooldownDuration: time.Second * time.Duration(cooldownDuration),
ShowJoinQuit: showJoinQuit,
MaxNickLength: maxNickLength,

Debug: *debugMode,
DebugPresence: *debugPresence,
Expand Down