Merge lp:~widelands-dev/widelands-metaserver/ircbridge into lp:widelands-metaserver

[Tino]

A first draft of a irce bridge. Lets the metaserver connect to a irc channel an mirrors messages between the channel and the metaserver lobby.

[SirVer (sirver)]

Nice. That is a really good idea and I think the implementation looks reasonable too. It is a bit drafty still, though (see comments below). Was this the first time you did something with go? How did you like it?

Some comments:

- func NewIRCBridge() IRCBridge should read its configuration from the same json file where we read the data from right now too (see main.go).
- I think the design is a little fragile - if something goes wrong with the IRC connection the whole metaserver comes down. How about running the IRC bridge in a go routine that gets a (buffered) channel of stuff to send and also sends stuff into a channel that the metaserver then displays? That way if the bridge is disconnected it can just discard the messages and the whole design is a bit decoupled. Also you can fake some IRC messages in a test and test the whole thing.
- +func (s *Server) Motd() string - I do not agree with this change. When a method is not changing a struct, it should not be a pointer receiver. There are only a few exceptions to this rule as far as I understood go so far (not an expert by any means).
- Please add some tests. Feel free to add end to end tests, there are already a few in the metaserver.

[SirVer (sirver)]

review: Needs Fixing

[Tino (tino79)]

Thanks for the comments, of course i only wanted a review, not a merge in the current state.
This are my first steps with go, so i am just glad that it works at all ;).

- i will add tests
- the irc library handles disconnects itself, and because of the use of a callback function for irc->lobby it is already decoupled. I disconnected from the internet to test and it automatically reconnected and the lobby was still usable. But i will look into using a routine and buffers...
- pointer recievers:
Here i am really unsure, but i've found:

   A method is a function with an implicit first argument, called a receiver.

   So when you call a method:

   - if the receiver is a pointer to int, it copies the pointer to int.
   - if the receiver is a struct, it copies the struct.
   - if the receiver is a pointer to struct, it copies the pointer to struct.

So even if i don't want to modify the struct i would think avoiding a copy operation by passing a pointer to the struct should be faster? But here speaks only 1 day go experience... ;)

[SirVer (sirver)]

> But i will look into using a routine and buffers...

I understand your comment, but I still think it is a good idea. First, because it is more idiomatic go (as far as I see it right now :)) and secondly goroutines and channels are essential building blocks that somehow set the language apart - you'll find them really interesting I think.

> So even if i don't want to modify the struct i would think avoiding a copy operation by passing a pointer to the struct should be faster?

While this is correct, go optimizes these very strongly. Sofar not using a pointer has not been a performance bottleneck in go code for me. And not using a pointer signifies to the programmer that the method will not change the receiver - which is nice documentation. So only use a pointer receiver when you need to (i.e. when you need to modify the receiver) or when you measured that it impacts performance by benchmarking.

[lp:~widelands-dev/widelands-metaserver/ircbridge]

49. By Tino on 2014-01-27

revert to non pointer recievers when the struct does not get manipulated

50. By Tino on 2014-01-27

read irc configuration from file

51. By Tino on 2014-01-28

IRCBridge "interfaced", does compile and run again

52. By Tino on 2014-01-28

pushing messages to irc via channel

53. By Tino on 2014-01-28

go channels 2way irc<->lobby

54. By Tino on 2014-01-29

channel refactoring
tests are ok again (no testing of irc functionality right now)

55. By Tino on 2014-01-29

non blocking sending

[Tino (tino79)]

Ok, channels are in place.
Next up are tests, but first i need to understand go tests. The current tests starts dozens of server instances in a few seconds, i need to use only one irc bridge instance for all tests or have to implement my own tests with active irc bridge.

[SirVer (sirver)]

I do not fully understand your question.

If you only want to test the behavior of the server you can just hand in two channels and fake messages on those in your server tests (i.e. no IRC Bridge is created at all). I think this is the most reasonable approach to take since you cannot rely on IRC being up when you want to test your scripts.

You can also choose not to test your new implementations, but you must at least change the setup procedure so that not necessarily an IRC connection is made when a server is created.

[SirVer (sirver)]

And progress on this? Anything I can help you with? If you are stuck we can also remote pair on the code for a bit, maybe we come up with something together.

[lp:~widelands-dev/widelands-metaserver/ircbridge]

56. By Tino on 2014-02-04

correct irc address

57. By Tino on 2014-02-04

merge trunk

[Tino (tino79)]

The current state is:
- it is working, communication between ircbridge and metaserver via nonblocking channels
- tests are working, result ok
- no tests regarding the ircbridge itself or communication implemented

I am a bit busy atm, i think i won't have time to implement further test before next week.

[SirVer (sirver)]

no pressure - just wondered if you still want to work on this or if it should be merged around now.

[lp:~widelands-dev/widelands-metaserver/ircbridge]

58. By Tino on 2014-03-17

merge trunk

59. By Tino on 2014-03-17

adapt to changes in irc library

60. By Tino on 2014-03-17

implement test draft

61. By Tino on 2014-03-17

join channel only after hello from server

[Tino (tino79)]

Ok, perhaps we can merge now?
I does work(tm), only the testcase i've drafted seems to do nothing?

review: Needs Resubmitting

[SirVer (sirver)]

looks good to me. I merged and deployed on the server - thanks for your work on this!

=== added file '.bzrignore'
--- .bzrignore	1970-01-01 00:00:00 +0000
+++ .bzrignore	2014-03-17 19:05:27 +0000
@@ -0,0 +1,1 @@
+*.exe

=== modified file 'wlms/client.go'
--- wlms/client.go	2014-02-03 21:14:19 +0000
+++ wlms/client.go	2014-03-17 19:05:27 +0000
@@ -277,6 +277,7 @@
 
 	if len(receiver) == 0 {
 		server.BroadcastToConnectedClients("CHAT", client.Name(), message, "public")
+		server.BroadcastToIrc(client.Name() + ": " + message)
 	} else {
 		recv_client := server.HasClient(receiver)
 		if recv_client != nil {

=== added file 'wlms/ircbridge.go'
--- wlms/ircbridge.go	1970-01-01 00:00:00 +0000
+++ wlms/ircbridge.go	2014-03-17 19:05:27 +0000
@@ -0,0 +1,71 @@
+package main
+
+import (
+	"log"
+
+	"github.com/thoj/go-ircevent"
+)
+
+type IRCBridger interface {
+	Connect(chan Message, chan Message) bool
+	Quit()
+}
+
+type IRCBridge struct {
+	connection                  *irc.Connection
+	nick, user, channel, server string
+	useTLS                      bool
+}
+
+type Message struct {
+	message, nick string
+}
+
+func NewIRCBridge(server, realname, nickname, channel string, tls bool) *IRCBridge {
+	return &IRCBridge{
+		server:  server,
+		user:    realname,
+		nick:    nickname,
+		channel: channel,
+		useTLS:  tls,
+	}
+}
+
+func (bridge *IRCBridge) Connect(messagesIn chan Message, messagesOut chan Message) bool {
+	//Create new connection
+	bridge.connection = irc.IRC(bridge.nick, bridge.user)
+	//Set options
+	bridge.connection.UseTLS = bridge.useTLS
+	//connection.TLSOptions //set ssl options
+	//connection.Password = "[server password]"
+	//Commands
+	err := bridge.connection.Connect(bridge.server) //Connect to server
+	if err != nil {
+		log.Fatal("Can't connect %s", bridge.server)
+		return false
+	}
+	bridge.connection.Join(bridge.channel)
+	bridge.connection.AddCallback("PRIVMSG", func(event *irc.Event) {
+		//e.Message contains the message
+		//e.Nick Contains the sender
+		//e.Arguments[0] Contains the channel
+		select {
+		case messagesOut <- Message{nick: event.Nick,
+			message: event.Message(),
+		}:
+		default:
+			log.Println("Message Queue full.")
+		}
+
+	})
+	go func() {
+		for m := range messagesIn {
+			bridge.connection.Privmsg(bridge.channel, m.message)
+		}
+	}()
+	return true
+}
+
+func (bridge *IRCBridge) Quit() {
+	bridge.connection.Quit()
+}

=== modified file 'wlms/main.go'
--- wlms/main.go	2013-12-30 12:20:31 +0000
+++ wlms/main.go	2014-03-17 19:05:27 +0000
@@ -8,7 +8,8 @@
 )
 
 type Config struct {
-	Database, User, Password, Table string
+	Database, User, Password, Table, Backend, IRCServer, Nickname, Realname, Channel string
+	UseTLS                                                                           bool
 }
 
 func (l *Config) ConfigFrom(path string) error {
@@ -25,16 +26,26 @@
 	flag.Parse()
 
 	var db UserDb
+	var ircbridge IRCBridger
 	if config != "" {
 		var cfg Config
 		if err := cfg.ConfigFrom(config); err != nil {
 			log.Fatalf("Could not parse config file: %v", err)
 		}
-		db = NewMySqlDatabase(cfg.Database, cfg.User, cfg.Password, cfg.Table)
+		if cfg.Backend == "mysql" {
+			db = NewMySqlDatabase(cfg.Database, cfg.User, cfg.Password, cfg.Table)
+		} else {
+			db = NewInMemoryDb()
+		}
+		ircbridge = NewIRCBridge(cfg.IRCServer, cfg.Realname, cfg.Nickname, cfg.Channel, cfg.UseTLS)
 	} else {
 		db = NewInMemoryDb()
 	}
 	defer db.Close()
 
-	RunServer(db)
+	messagesToIrc := make(chan Message, 50)
+	messagesToLobby := make(chan Message, 50)
+	ircbridge.Connect(messagesToIrc, messagesToLobby)
+	RunServer(db, messagesToLobby, messagesToIrc)
+
 }

=== modified file 'wlms/server.go'
--- wlms/server.go	2014-02-08 20:48:04 +0000
+++ wlms/server.go	2014-03-17 19:05:27 +0000
@@ -33,10 +33,11 @@
 	gamePingTimeout time.Duration
 
 	clientForgetTimeout time.Duration
-	gamePingerFactory   GampPingerFactory
+	gamePingerFactory   GamePingerFactory
+	messagesOut         chan Message
 }
 
-type GampPingerFactory interface {
+type GamePingerFactory interface {
 	New(client *Client, timeout time.Duration) *GamePinger
 }
 
@@ -100,6 +101,7 @@
 }
 
 func (s *Server) AddClient(client *Client) {
+	s.BroadcastToIrc(client.Name() + " has joined the lobby.")
 	s.clients.PushBack(client)
 }
 
@@ -121,11 +123,15 @@
 		if e.Value.(*Client) == client {
 			log.Printf("Removing client %s.", client.Name())
 			s.clients.Remove(e)
+			s.messagesOut <- Message{
+				message: client.Name() + " has left the lobby.",
+				nick:    client.Name(),
+			}
 		}
 	}
 }
 
-func (s *Server) HasClient(name string) *Client {
+func (s Server) HasClient(name string) *Client {
 	for e := s.clients.Front(); e != nil; e = e.Next() {
 		client := e.Value.(*Client)
 		if client.Name() == name {
@@ -135,7 +141,7 @@
 	return nil
 }
 
-func (s *Server) NrActiveClients() int {
+func (s Server) NrActiveClients() int {
 	count := 0
 	for e := s.clients.Front(); e != nil; e = e.Next() {
 		if e.Value.(*Client).State() == CONNECTED {
@@ -158,6 +164,7 @@
 func (s *Server) AddGame(game *Game) {
 	s.games.PushBack(game)
 	s.BroadcastToConnectedClients("GAMES_UPDATE")
+	s.BroadcastToIrc("A new game " + game.Name() + " was opened by " + game.Host())
 }
 
 func (s *Server) RemoveGame(game *Game) {
@@ -170,7 +177,7 @@
 	}
 }
 
-func (s *Server) HasGame(name string) *Game {
+func (s Server) HasGame(name string) *Game {
 	for e := s.games.Front(); e != nil; e = e.Next() {
 		game := e.Value.(*Game)
 		if game.Name() == name {
@@ -180,7 +187,7 @@
 	return nil
 }
 
-func (s *Server) NrGames() int {
+func (s Server) NrGames() int {
 	return s.games.Len()
 }
 
@@ -199,7 +206,18 @@
 	}
 }
 
-func RunServer(db UserDb) {
+func (s Server) BroadcastToIrc(message string) {
+	select {
+	case s.messagesOut <- Message{
+		message: message,
+	}:
+	default:
+		log.Println("Message Queue full.")
+	}
+
+}
+
+func RunServer(db UserDb, messagesIn chan Message, messagesOut chan Message) {
 	ln, err := net.Listen("tcp", ":7395")
 	if err != nil {
 		log.Fatal(err)
@@ -216,8 +234,7 @@
 			C <- conn
 		}
 	}()
-
-	CreateServerUsing(C, db).WaitTillShutdown()
+	CreateServerUsing(C, db, messagesIn, messagesOut).WaitTillShutdown()
 }
 
 type RealGamePingerFactory struct {
@@ -255,11 +272,11 @@
 	return pinger
 }
 
-func (server *Server) InjectGamePingerFactory(gpf GampPingerFactory) {
+func (server *Server) InjectGamePingerFactory(gpf GamePingerFactory) {
 	server.gamePingerFactory = gpf
 }
 
-func CreateServerUsing(acceptedConnections chan ReadWriteCloserWithIp, db UserDb) *Server {
+func CreateServerUsing(acceptedConnections chan ReadWriteCloserWithIp, db UserDb, messagesIn chan Message, messagesOut chan Message) *Server {
 	server := &Server{
 		acceptedConnections:    acceptedConnections,
 		shutdownServer:         make(chan bool),
@@ -272,8 +289,14 @@
 		pingCycleTime:          time.Second * 15,
 		clientSendingTimeout:   time.Minute * 2,
 		clientForgetTimeout:    time.Minute * 5,
+		messagesOut:            messagesOut,
 	}
 	server.gamePingerFactory = RealGamePingerFactory{server}
+	go func() {
+		for m := range messagesIn {
+			server.BroadcastToConnectedClients("CHAT", m.nick+"(IRC)", m.message, "public")
+		}
+	}()
 	go server.mainLoop()
 	return server
 }

=== modified file 'wlms/server_test.go'
--- wlms/server_test.go	2014-02-08 20:48:04 +0000
+++ wlms/server_test.go	2014-03-17 19:05:27 +0000
@@ -40,7 +40,12 @@
 		cons[i] = NewFakeConn(c)
 		acceptingConnections <- cons[i]
 	}
-	return CreateServerUsing(acceptingConnections, db), cons
+
+	//irc := NewIRCBridge("chat.freenode.net:7000", "wltest", "wltest", "#widelands-test", true)
+	toIrc := make(chan Message, 100)
+	fromIrc := make(chan Message, 100)
+	//irc.Connect(toIrc, fromIrc)
+	return CreateServerUsing(acceptingConnections, db, fromIrc, toIrc), cons
 }
 
 type Matching string