GODT-1159 SMTP server not restarting after restored internet

- [x] write tests to check that IMAP and SMTP servers are closed when there
  is no internet
- [x] always create new go-smtp instance during listenAndServe(int)

Makefile

@@ -253,6 +253,9 @@ bench:
 coverage: test
 	go tool cover -html=/tmp/coverage.out -o=coverage.html
 
+integration-test-bridge:
+	${MAKE} -C test test-bridge
+
 mocks:
 	mockgen --package mocks github.com/ProtonMail/proton-bridge/internal/users Locator,PanicHandler,CredentialsStorer,StoreMaker > internal/users/mocks/mocks.go
 	mockgen --package mocks github.com/ProtonMail/proton-bridge/pkg/listener Listener > internal/users/mocks/listener_mocks.go

internal/imap/server.go

@@ -32,8 +32,8 @@ import (
 	"github.com/ProtonMail/proton-bridge/internal/events"
 	"github.com/ProtonMail/proton-bridge/internal/imap/id"
 	"github.com/ProtonMail/proton-bridge/internal/imap/uidplus"
+	"github.com/ProtonMail/proton-bridge/internal/serverutil"
 	"github.com/ProtonMail/proton-bridge/pkg/listener"
-	"github.com/ProtonMail/proton-bridge/pkg/ports"
 	"github.com/emersion/go-imap"
 	imapappendlimit "github.com/emersion/go-imap-appendlimit"
 	imapidle "github.com/emersion/go-imap-idle"
@@ -116,18 +116,20 @@ func NewIMAPServer(panicHandler panicHandler, debugClient, debugServer bool, por
 	return server
 }
 
-// Starts the server.
+func (s *imapServer) HandlePanic()    { s.panicHandler.HandlePanic() }
-func (s *imapServer) ListenAndServe() {
+func (s *imapServer) IsRunning() bool { return s.isRunning.Load().(bool) }
-	go s.monitorDisconnectedUsers()
+func (s *imapServer) Port() int       { return s.port }
-	go s.monitorInternetConnection()
 
-	// When starting the Bridge, we don't want to retry to notify user
+// ListenAndServe starts the server and keeps it on based on internet
-	// quickly about the issue. Very probably retry will not help anyway.
+// availability.
-	s.listenAndServe(0)
+func (s *imapServer) ListenAndServe() {
+	serverutil.ListenAndServe(s, s.eventListener)
 }
 
-func (s *imapServer) listenAndServe(retries int) {
+// ListenRetryAndServe will start listener. If port is occupied it will try
-	if s.isRunning.Load().(bool) {
+// again after coolDown time. Once listener is OK it will serve.
+func (s *imapServer) ListenRetryAndServe(retries int, retryAfter time.Duration) {
+	if s.IsRunning() {
 		return
 	}
 	s.isRunning.Store(true)
@@ -139,8 +141,8 @@ func (s *imapServer) listenAndServe(retries int) {
 		s.isRunning.Store(false)
 		if retries > 0 {
 			l.WithError(err).WithField("retries", retries).Warn("IMAP listener failed")
-			time.Sleep(15 * time.Second)
+			time.Sleep(retryAfter)
-			s.listenAndServe(retries - 1)
+			s.ListenRetryAndServe(retries-1, retryAfter)
 			return
 		}
 
@@ -157,7 +159,7 @@ func (s *imapServer) listenAndServe(retries int) {
 	// Serve returns error every time, even after closing the server.
 	// User shouldn't be notified about error if server shouldn't be running,
 	// but it should in case it was not closed by `s.Close()`.
-	if err != nil && s.isRunning.Load().(bool) {
+	if err != nil && s.IsRunning() {
 		s.isRunning.Store(false)
 		l.WithError(err).Error("IMAP server failed")
 		s.eventListener.Emit(events.ErrorEvent, "IMAP failed: "+err.Error())
@@ -170,7 +172,7 @@ func (s *imapServer) listenAndServe(retries int) {
 
 // Stops the server.
 func (s *imapServer) Close() {
-	if !s.isRunning.Load().(bool) {
+	if !s.IsRunning() {
 		return
 	}
 	s.isRunning.Store(false)
@@ -181,62 +183,16 @@ func (s *imapServer) Close() {
 	}
 }
 
-func (s *imapServer) monitorInternetConnection() {
+func (s *imapServer) DisconnectUser(address string) {
-	on := make(chan string)
+	log.Info("Disconnecting all open IMAP connections for ", address)
-	s.eventListener.Add(events.InternetOnEvent, on)
+	s.server.ForEachConn(func(conn imapserver.Conn) {
-	off := make(chan string)
+		connUser := conn.Context().User
-	s.eventListener.Add(events.InternetOffEvent, off)
+		if connUser != nil && strings.EqualFold(connUser.Username(), address) {
-
+			if err := conn.Close(); err != nil {
-	for {
+				log.WithError(err).Error("Failed to close the connection")
-		var expectedIsPortFree bool
-		select {
-		case <-on:
-			go func() {
-				defer s.panicHandler.HandlePanic()
-				// We had issues on Mac that from time to time something
-				// blocked our port for a bit after we closed IMAP server
-				// due to connection issues.
-				// Restart always helped, so we do retry to not bother user.
-				s.listenAndServe(10)
-			}()
-			expectedIsPortFree = false
-		case <-off:
-			s.Close()
-			expectedIsPortFree = true
-		}
-
-		start := time.Now()
-		for {
-			if ports.IsPortFree(s.port) == expectedIsPortFree {
-				break
-			}
-			// Safety stop if something went wrong.
-			if time.Since(start) > 15*time.Second {
-				log.WithField("expectedIsPortFree", expectedIsPortFree).Warn("Server start/stop check timeouted")
-				break
-			}
-			time.Sleep(100 * time.Millisecond)
-		}
-	}
-}
-
-func (s *imapServer) monitorDisconnectedUsers() {
-	ch := make(chan string)
-	s.eventListener.Add(events.CloseConnectionEvent, ch)
-
-	for address := range ch {
-		address := address
-		log.Info("Disconnecting all open IMAP connections for ", address)
-		disconnectUser := func(conn imapserver.Conn) {
-			connUser := conn.Context().User
-			if connUser != nil && strings.EqualFold(connUser.Username(), address) {
-				if err := conn.Close(); err != nil {
-					log.WithError(err).Error("Failed to close the connection")
-				}
 			}
 		}
-		s.server.ForEachConn(disconnectUser)
+	})
-	}
 }
 
 // connListener sets debug loggers on server containing fields with local

internal/imap/server_test.go

@@ -20,48 +20,33 @@ package imap
 import (
 	"fmt"
 	"testing"
-	"time"
 
 	"github.com/ProtonMail/proton-bridge/internal/bridge"
 	"github.com/ProtonMail/proton-bridge/internal/config/useragent"
-	"github.com/ProtonMail/proton-bridge/internal/events"
+	"github.com/ProtonMail/proton-bridge/internal/serverutil/mocks"
-	"github.com/ProtonMail/proton-bridge/pkg/listener"
-	"github.com/ProtonMail/proton-bridge/pkg/ports"
 	imapserver "github.com/emersion/go-imap/server"
 
 	"github.com/stretchr/testify/require"
 )
 
-type testPanicHandler struct{}
-
-func (ph *testPanicHandler) HandlePanic() {}
-
 func TestIMAPServerTurnOffAndOnAgain(t *testing.T) {
-	panicHandler := &testPanicHandler{}
+	r := require.New(t)
+	ts := mocks.NewTestServer(12345)
 
-	eventListener := listener.New()
-
-	port := ports.FindFreePortFrom(12345)
 	server := imapserver.New(nil)
-	server.Addr = fmt.Sprintf("%v:%v", bridge.Host, port)
+	server.Addr = fmt.Sprintf("%v:%v", bridge.Host, ts.WantPort)
 
 	s := &imapServer{
-		panicHandler:  panicHandler,
+		panicHandler:  ts.PanicHandler,
 		server:        server,
-		eventListener: eventListener,
+		port:          ts.WantPort,
+		eventListener: ts.EventListener,
 		userAgent:     useragent.New(),
 	}
 	s.isRunning.Store(false)
 
+	r.True(ts.IsPortFree())
+
 	go s.ListenAndServe()
-	time.Sleep(5 * time.Second)
+	ts.RunServerTests(r)
-	require.False(t, ports.IsPortFree(port))
-
-	eventListener.Emit(events.InternetOffEvent, "")
-	time.Sleep(10 * time.Second)
-	require.True(t, ports.IsPortFree(port))
-
-	eventListener.Emit(events.InternetOnEvent, "")
-	time.Sleep(10 * time.Second)
-	require.False(t, ports.IsPortFree(port))
 }

internal/serverutil/mocks/server.go

@@ -0,0 +1,150 @@
+// Copyright (c) 2021 Proton Technologies AG
+//
+// This file is part of ProtonMail Bridge.
+//
+// ProtonMail Bridge is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// ProtonMail Bridge is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with ProtonMail Bridge.  If not, see <https://www.gnu.org/licenses/>.
+
+package mocks
+
+import (
+	"fmt"
+	"net/http"
+	"sync/atomic"
+	"time"
+
+	"github.com/ProtonMail/proton-bridge/internal/events"
+	"github.com/ProtonMail/proton-bridge/pkg/listener"
+	"github.com/ProtonMail/proton-bridge/pkg/ports"
+	"github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/require"
+)
+
+type DummyPanicHandler struct{}
+
+func (ph *DummyPanicHandler) HandlePanic() {}
+
+type TestServer struct {
+	PanicHandler  *DummyPanicHandler
+	WantPort      int
+	EventListener listener.Listener
+
+	isRunning atomic.Value
+	srv       *http.Server
+}
+
+func NewTestServer(port int) *TestServer {
+	s := &TestServer{
+		PanicHandler:  &DummyPanicHandler{},
+		EventListener: listener.New(),
+		WantPort:      ports.FindFreePortFrom(port),
+	}
+	s.isRunning.Store(false)
+	return s
+}
+
+func (s *TestServer) IsPortFree() bool {
+	return true
+}
+
+func (s *TestServer) IsPortOccupied() bool {
+	return true
+}
+
+func (s *TestServer) Emit(event string, try, iEvt int) int {
+	// Emit has separate go routine so it is needed to wait here to
+	// prevent event race condition.
+	time.Sleep(100 * time.Millisecond)
+	iEvt++
+	s.EventListener.Emit(event, fmt.Sprintf("%d:%d", try, iEvt))
+	return iEvt
+}
+
+func (s *TestServer) HandlePanic()          {}
+func (s *TestServer) DisconnectUser(string) {}
+func (s *TestServer) Port() int             { return s.WantPort }
+func (s *TestServer) IsRunning() bool       { return s.isRunning.Load().(bool) }
+
+func (s *TestServer) ListenRetryAndServe(retries int, retryAfter time.Duration) {
+	if s.isRunning.Load().(bool) {
+		return
+	}
+	s.isRunning.Store(true)
+
+	// There can be delay when starting server
+	time.Sleep(200 * time.Millisecond)
+
+	s.srv = &http.Server{
+		Addr: fmt.Sprintf("127.0.0.1:%d", s.WantPort),
+	}
+
+	err := s.srv.ListenAndServe()
+	if err != nil {
+		s.isRunning.Store(false)
+		if retries > 0 {
+			time.Sleep(retryAfter)
+			s.ListenRetryAndServe(retries-1, retryAfter)
+		}
+	}
+
+	if s.IsRunning() {
+		logrus.Error("Not serving but isRunning is true")
+		s.isRunning.Store(false)
+	}
+}
+
+func (s *TestServer) Close() {
+	if !s.isRunning.Load().(bool) {
+		return
+	}
+	s.isRunning.Store(false)
+
+	// There can be delay when stopping server
+	time.Sleep(200 * time.Millisecond)
+	if err := s.srv.Close(); err != nil {
+		logrus.WithError(err).Error("Closing dummy server")
+	}
+}
+
+func (s *TestServer) RunServerTests(r *require.Assertions) {
+	// NOTE About choosing tick durations:
+	// In order to avoid ticks to synchronise and cause occasional race
+	// condition we choose the tick duration around 100ms but not exactly
+	// to have large common multiple.
+	r.Eventually(s.IsPortOccupied, 5*time.Second, 97*time.Millisecond)
+
+	// There was an issue where second time we were not able to restore server.
+	for try := 0; try < 3; try++ {
+		i := s.Emit(events.InternetOffEvent, try, 0)
+		r.Eventually(s.IsPortFree, 10*time.Second, 99*time.Millisecond, "signal off try %d : %d", try, i)
+
+		i = s.Emit(events.InternetOnEvent, try, i)
+		i = s.Emit(events.InternetOffEvent, try, i)
+		i = s.Emit(events.InternetOffEvent, try, i)
+		i = s.Emit(events.InternetOffEvent, try, i)
+		i = s.Emit(events.InternetOffEvent, try, i)
+		i = s.Emit(events.InternetOnEvent, try, i)
+		i = s.Emit(events.InternetOnEvent, try, i)
+		i = s.Emit(events.InternetOffEvent, try, i)
+		// Wait a bit longer if needed to process all events
+		r.Eventually(s.IsPortFree, 20*time.Second, 101*time.Millisecond, "again signal off number %d : %d", try, i)
+
+		i = s.Emit(events.InternetOnEvent, try, i)
+		r.Eventually(s.IsPortOccupied, 10*time.Second, 103*time.Millisecond, "signal on number %d : %d", try, i)
+
+		i = s.Emit(events.InternetOffEvent, try, i)
+		i = s.Emit(events.InternetOnEvent, try, i)
+		i = s.Emit(events.InternetOnEvent, try, i)
+		r.Eventually(s.IsPortOccupied, 10*time.Second, 107*time.Millisecond, "again signal on number %d : %d", try, i)
+	}
+}

internal/serverutil/server.go

@@ -0,0 +1,132 @@
+// Copyright (c) 2021 Proton Technologies AG
+//
+// This file is part of ProtonMail Bridge.
+//
+// ProtonMail Bridge is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// ProtonMail Bridge is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with ProtonMail Bridge.  If not, see <https://www.gnu.org/licenses/>.
+
+package serverutil
+
+import (
+	"time"
+
+	"github.com/ProtonMail/proton-bridge/internal/events"
+	"github.com/ProtonMail/proton-bridge/pkg/listener"
+	"github.com/ProtonMail/proton-bridge/pkg/ports"
+	"github.com/sirupsen/logrus"
+)
+
+// Server which can handle disconnected users and lost internet connection.
+type Server interface {
+	HandlePanic()
+	DisconnectUser(string)
+	ListenRetryAndServe(int, time.Duration)
+	Close()
+	Port() int
+	IsRunning() bool
+}
+
+func monitorDisconnectedUsers(s Server, l listener.Listener) {
+	ch := make(chan string)
+	l.Add(events.CloseConnectionEvent, ch)
+	for address := range ch {
+		s.DisconnectUser(address)
+	}
+}
+
+func redirectInternetEventsToOneChannel(l listener.Listener) (isInternetOn chan bool) {
+	on := make(chan string)
+	l.Add(events.InternetOnEvent, on)
+	off := make(chan string)
+	l.Add(events.InternetOffEvent, off)
+
+	// Redirect two channels into one. When select was used the algorithm
+	// first read all on channels and then read all off channels.
+	isInternetOn = make(chan bool, 20)
+	go func() {
+		for {
+			logrus.WithField("try", <-on).Trace("Internet ON")
+			isInternetOn <- true
+		}
+	}()
+
+	go func() {
+		for {
+			logrus.WithField("try", <-off).Trace("Internet OFF")
+			isInternetOn <- false
+		}
+	}()
+	return
+}
+
+const (
+	recheckPortAfter    = 50 * time.Millisecond
+	stopPortChecksAfter = 15 * time.Second
+	retryListnerAfter   = 5 * time.Second
+)
+
+func monitorInternetConnection(s Server, l listener.Listener) {
+	isInternetOn := redirectInternetEventsToOneChannel(l)
+	for {
+		var expectedIsPortFree bool
+		if <-isInternetOn {
+			if s.IsRunning() {
+				continue
+			}
+			go func() {
+				defer s.HandlePanic()
+				// We had issues on Mac that from time to time something
+				// blocked our port for a bit after we closed IMAP server
+				// due to connection issues.
+				// Restart always helped, so we do retry to not bother user.
+				s.ListenRetryAndServe(10, retryListnerAfter)
+			}()
+			expectedIsPortFree = false
+		} else {
+			if !s.IsRunning() {
+				continue
+			}
+			s.Close()
+			expectedIsPortFree = true
+		}
+		start := time.Now()
+		for {
+			isPortFree := ports.IsPortFree(s.Port())
+			logrus.
+				WithField("port", s.Port()).
+				WithField("isFree", isPortFree).
+				WithField("wantToBeFree", expectedIsPortFree).
+				Trace("Check port")
+			if isPortFree == expectedIsPortFree {
+				break
+			}
+			// Safety stop if something went wrong.
+			if time.Since(start) > stopPortChecksAfter {
+				logrus.WithField("expectedIsPortFree", expectedIsPortFree).Warn("Server start/stop check timeouted")
+				break
+			}
+			time.Sleep(recheckPortAfter)
+		}
+	}
+}
+
+// ListenAndServe starts the server and keeps it on based on internet
+// availability. It also monitors and disconnect users if requested.
+func ListenAndServe(s Server, l listener.Listener) {
+	go monitorDisconnectedUsers(s, l)
+	go monitorInternetConnection(s, l)
+
+	// When starting the Bridge, we don't want to retry to notify user
+	// quickly about the issue. Very probably retry will not help anyway.
+	s.ListenRetryAndServe(0, 0)
+}

internal/serverutil/server_test.go

@@ -0,0 +1,35 @@
+// Copyright (c) 2021 Proton Technologies AG
+//
+// This file is part of ProtonMail Bridge.
+//
+// ProtonMail Bridge is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// ProtonMail Bridge is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with ProtonMail Bridge.  If not, see <https://www.gnu.org/licenses/>.
+
+package serverutil
+
+import (
+	"testing"
+
+	"github.com/ProtonMail/proton-bridge/internal/serverutil/mocks"
+	"github.com/stretchr/testify/require"
+)
+
+func TestServerTurnOffAndOnAgain(t *testing.T) {
+	r := require.New(t)
+	s := mocks.NewTestServer(12321)
+
+	r.True(s.IsPortFree())
+
+	go ListenAndServe(s, s.EventListener)
+	s.RunServerTests(r)
+}

internal/smtp/server.go

@@ -26,31 +26,28 @@ import (
 
 	"github.com/ProtonMail/proton-bridge/internal/bridge"
 	"github.com/ProtonMail/proton-bridge/internal/events"
+	"github.com/ProtonMail/proton-bridge/internal/serverutil"
 	"github.com/ProtonMail/proton-bridge/pkg/listener"
-	"github.com/ProtonMail/proton-bridge/pkg/ports"
 	"github.com/emersion/go-sasl"
 	goSMTP "github.com/emersion/go-smtp"
 	"github.com/sirupsen/logrus"
 )
 
-type smtpServer struct {
+// Server is Bridge SMTP server implementation.
+type Server struct {
 	panicHandler  panicHandler
+	backend       goSMTP.Backend
 	server        *goSMTP.Server
 	eventListener listener.Listener
+	debug         bool
 	useSSL        bool
 	port          int
+	tls           *tls.Config
 	isRunning     atomic.Value
 }
 
 // NewSMTPServer returns an SMTP server configured with the given options.
-func NewSMTPServer(panicHandler panicHandler, debug bool, port int, useSSL bool, tls *tls.Config, smtpBackend goSMTP.Backend, eventListener listener.Listener) *smtpServer { //nolint[golint]
+func NewSMTPServer(panicHandler panicHandler, debug bool, port int, useSSL bool, tls *tls.Config, smtpBackend goSMTP.Backend, eventListener listener.Listener) *Server {
-	s := goSMTP.NewServer(smtpBackend)
-	s.Addr = fmt.Sprintf("%v:%v", bridge.Host, port)
-	s.TLSConfig = tls
-	s.Domain = bridge.Host
-	s.AllowInsecureAuth = true
-	s.MaxLineLength = 2 << 16
-
 	if debug {
 		fmt.Println("THE LOG WILL CONTAIN **DECRYPTED** MESSAGE DATA")
 		log.Warning("================================================")
@@ -58,13 +55,38 @@ func NewSMTPServer(panicHandler panicHandler, debug bool, port int, useSSL bool,
 		log.Warning("================================================")
 	}
 
+	server := &Server{
+		panicHandler:  panicHandler,
+		backend:       smtpBackend,
+		eventListener: eventListener,
+		debug:         debug,
+		useSSL:        useSSL,
+		port:          port,
+		tls:           tls,
+	}
+	server.isRunning.Store(false)
+	return server
+}
+
+func (s *Server) HandlePanic()    { s.panicHandler.HandlePanic() }
+func (s *Server) IsRunning() bool { return s.isRunning.Load().(bool) }
+func (s *Server) Port() int       { return s.port }
+
+func newGoSMTPServer(debug bool, smtpBackend goSMTP.Backend, port int, tls *tls.Config) *goSMTP.Server {
+	newSMTP := goSMTP.NewServer(smtpBackend)
+	newSMTP.Addr = fmt.Sprintf("%v:%v", bridge.Host, port)
+	newSMTP.TLSConfig = tls
+	newSMTP.Domain = bridge.Host
+	newSMTP.AllowInsecureAuth = true
+	newSMTP.MaxLineLength = 1 << 16
+
 	if debug {
-		s.Debug = logrus.
+		newSMTP.Debug = logrus.
 			WithField("pkg", "smtp/server").
 			WriterLevel(logrus.DebugLevel)
 	}
 
-	s.EnableAuth(sasl.Login, func(conn *goSMTP.Conn) sasl.Server {
+	newSMTP.EnableAuth(sasl.Login, func(conn *goSMTP.Conn) sasl.Server {
 		return sasl.NewLoginServer(func(address, password string) error {
 			user, err := conn.Server().Backend.Login(nil, address, password)
 			if err != nil {
@@ -75,36 +97,26 @@ func NewSMTPServer(panicHandler panicHandler, debug bool, port int, useSSL bool,
 			return nil
 		})
 	})
-
+	return newSMTP
-	server := &smtpServer{
-		panicHandler:  panicHandler,
-		server:        s,
-		eventListener: eventListener,
-		useSSL:        useSSL,
-		port:          port,
-	}
-	server.isRunning.Store(false)
-	return server
 }
 
-// Starts the server.
+// ListenAndServe starts the server and keeps it on based on internet
-func (s *smtpServer) ListenAndServe() {
+// availability.
-	go s.monitorDisconnectedUsers()
+func (s *Server) ListenAndServe() {
-	go s.monitorInternetConnection()
+	serverutil.ListenAndServe(s, s.eventListener)
-
-	// When starting the Bridge, we don't want to retry to notify user
-	// quickly about the issue. Very probably retry will not help anyway.
-	s.listenAndServe(0)
 }
 
-func (s *smtpServer) listenAndServe(retries int) {
+func (s *Server) ListenRetryAndServe(retries int, retryAfter time.Duration) {
-	if s.isRunning.Load().(bool) {
+	if s.IsRunning() {
 		return
 	}
 	s.isRunning.Store(true)
 
+	s.server = newGoSMTPServer(s.debug, s.backend, s.port, s.tls)
+
 	l := log.WithField("useSSL", s.useSSL).WithField("address", s.server.Addr)
 	l.Info("SMTP server is starting")
+
 	var listener net.Listener
 	var err error
 	if s.useSSL {
@@ -112,12 +124,13 @@ func (s *smtpServer) listenAndServe(retries int) {
 	} else {
 		listener, err = net.Listen("tcp", s.server.Addr)
 	}
+	l.WithError(err).Debug("Listener for SMTP created")
 	if err != nil {
 		s.isRunning.Store(false)
 		if retries > 0 {
 			l.WithError(err).WithField("retries", retries).Warn("SMTP listener failed")
-			time.Sleep(15 * time.Second)
+			time.Sleep(retryAfter)
-			s.listenAndServe(retries - 1)
+			s.ListenRetryAndServe(retries-1, retryAfter)
 			return
 		}
 
@@ -127,85 +140,49 @@ func (s *smtpServer) listenAndServe(retries int) {
 	}
 
 	err = s.server.Serve(listener)
+	l.WithError(err).Debug("GoSMTP not serving")
 	// Serve returns error every time, even after closing the server.
 	// User shouldn't be notified about error if server shouldn't be running,
 	// but it should in case it was not closed by `s.Close()`.
-	if err != nil && s.isRunning.Load().(bool) {
+	if err != nil && s.IsRunning() {
 		s.isRunning.Store(false)
 		l.WithError(err).Error("SMTP server failed")
 		s.eventListener.Emit(events.ErrorEvent, "SMTP failed: "+err.Error())
 		return
 	}
-	defer s.server.Close() //nolint[errcheck]
+	defer func() {
+		// Go SMTP server instance can be closed only once. Otherwise
+		// it returns an error. The error is not export therefore we
+		// will check the string value.
+		err := s.server.Close()
+		if err == nil || err.Error() != "smtp: server already closed" {
+			l.WithError(err).Warn("Server was not closed")
+		}
+	}()
 
-	l.Info("SMTP server stopped")
+	l.Info("SMTP server closed")
 }
 
-// Stops the server.
+// Close stops the server.
-func (s *smtpServer) Close() {
+func (s *Server) Close() {
-	if !s.isRunning.Load().(bool) {
+	if !s.IsRunning() {
 		return
 	}
 	s.isRunning.Store(false)
 
 	if err := s.server.Close(); err != nil {
-		log.WithError(err).Error("Failed to close the connection")
+		log.WithError(err).Error("Cannot close the server")
 	}
 }
 
-func (s *smtpServer) monitorInternetConnection() {
+func (s *Server) DisconnectUser(address string) {
-	on := make(chan string)
+	log.Info("Disconnecting all open SMTP connections for ", address)
-	s.eventListener.Add(events.InternetOnEvent, on)
+	s.server.ForEachConn(func(conn *goSMTP.Conn) {
-	off := make(chan string)
+		connUser := conn.Session()
-	s.eventListener.Add(events.InternetOffEvent, off)
+		if connUser != nil {
-
+			if err := conn.Close(); err != nil {
-	for {
+				log.WithError(err).Error("Failed to close the connection")
-		var expectedIsPortFree bool
-		select {
-		case <-on:
-			go func() {
-				defer s.panicHandler.HandlePanic()
-				// We had issues on Mac that from time to time something
-				// blocked our port for a bit after we closed IMAP server
-				// due to connection issues.
-				// Restart always helped, so we do retry to not bother user.
-				s.listenAndServe(10)
-			}()
-			expectedIsPortFree = false
-		case <-off:
-			s.Close()
-			expectedIsPortFree = true
-		}
-
-		start := time.Now()
-		for {
-			if ports.IsPortFree(s.port) == expectedIsPortFree {
-				break
 			}
-			// Safety stop if something went wrong.
-			if time.Since(start) > 15*time.Second {
-				log.WithField("expectedIsPortFree", expectedIsPortFree).Warn("Server start/stop check timeouted")
-				break
-			}
-			time.Sleep(100 * time.Millisecond)
 		}
-	}
+	})
-}
-
-func (s *smtpServer) monitorDisconnectedUsers() {
-	ch := make(chan string)
-	s.eventListener.Add(events.CloseConnectionEvent, ch)
-
-	for address := range ch {
-		log.Info("Disconnecting all open SMTP connections for ", address)
-		disconnectUser := func(conn *goSMTP.Conn) {
-			connUser := conn.Session()
-			if connUser != nil {
-				if err := conn.Close(); err != nil {
-					log.WithError(err).Error("Failed to close the connection")
-				}
-			}
-		}
-		s.server.ForEachConn(disconnectUser)
-	}
 }

internal/smtp/server_test.go

@@ -18,48 +18,26 @@
 package smtp
 
 import (
-	"fmt"
 	"testing"
-	"time"
 
-	"github.com/ProtonMail/proton-bridge/internal/bridge"
+	"github.com/ProtonMail/proton-bridge/internal/serverutil/mocks"
-	"github.com/ProtonMail/proton-bridge/internal/events"
-	"github.com/ProtonMail/proton-bridge/pkg/listener"
-	"github.com/ProtonMail/proton-bridge/pkg/ports"
-	goSMTP "github.com/emersion/go-smtp"
 
 	"github.com/stretchr/testify/require"
 )
 
-type testPanicHandler struct{}
-
-func (ph *testPanicHandler) HandlePanic() {}
-
 func TestSMTPServerTurnOffAndOnAgain(t *testing.T) {
-	panicHandler := &testPanicHandler{}
+	r := require.New(t)
+	ts := mocks.NewTestServer(12342)
 
-	eventListener := listener.New()
+	s := &Server{
-
+		panicHandler:  ts.PanicHandler,
-	port := ports.FindFreePortFrom(12345)
+		port:          ts.WantPort,
-	server := goSMTP.NewServer(nil)
+		eventListener: ts.EventListener,
-	server.Addr = fmt.Sprintf("%v:%v", bridge.Host, port)
-
-	s := &smtpServer{
-		panicHandler:  panicHandler,
-		server:        server,
-		eventListener: eventListener,
 	}
 	s.isRunning.Store(false)
 
+	r.True(ts.IsPortFree())
+
 	go s.ListenAndServe()
-	time.Sleep(5 * time.Second)
+	ts.RunServerTests(r)
-	require.False(t, ports.IsPortFree(port))
-
-	eventListener.Emit(events.InternetOffEvent, "")
-	time.Sleep(10 * time.Second)
-	require.True(t, ports.IsPortFree(port))
-
-	eventListener.Emit(events.InternetOnEvent, "")
-	time.Sleep(10 * time.Second)
-	require.False(t, ports.IsPortFree(port))
 }

pkg/listener/listener.go

@@ -88,7 +88,9 @@ func (l *listener) Add(eventName string, channel chan<- string) {
 		l.channels = make(map[string][]chan<- string)
 	}
 
+	log := log.WithField("name", eventName).WithField("i", len(l.channels[eventName]))
 	l.channels[eventName] = append(l.channels[eventName], channel)
+	log.Debug("Added event listner")
 }
 
 // Remove removes an event listener.
@@ -123,8 +125,10 @@ func (l *listener) emit(eventName, data string, isReEmit bool) {
 	if _, ok := l.channels[eventName]; ok {
 		for i, handler := range l.channels[eventName] {
 			go func(handler chan<- string, i int) {
+				log := log.WithField("name", eventName).WithField("i", i).WithField("data", data)
+				log.Debug("Send event")
 				handler <- data
-				log.Debugf("emitted %s data %s -> %d", eventName, data, i)
+				log.Debug("Event sent")
 			}(handler, i)
 		}
 	} else if !isReEmit {

test/api_actions_test.go

@@ -26,7 +26,7 @@ import (
 func APIActionsFeatureContext(s *godog.Suite) {
 	s.Step(`^the internet connection is lost$`, theInternetConnectionIsLost)
 	s.Step(`^the internet connection is restored$`, theInternetConnectionIsRestored)
-	s.Step(`^(\d+) seconds pass$`, secondsPass)
+	s.Step(`^(\d+) second[s]? pass$`, secondsPass)
 }
 
 func theInternetConnectionIsLost() error {

test/features/bridge/no_internet.feature

@@ -0,0 +1,28 @@
+Feature: Servers are closed when no internet
+
+  Scenario: All connection are closed and then restored multiple times
+    Given there is connected user "user"
+    And there is IMAP client "i1" logged in as "user"
+    And there is SMTP client "s1" logged in as "user"
+    When there is no internet connection
+    And 1 second pass
+    Then IMAP client "i1" is logged out
+    And SMTP client "s1" is logged out
+    Given the internet connection is restored
+    And there is IMAP client "i2" logged in as "user"
+    And there is SMTP client "s2" logged in as "user"
+    When IMAP client "i2" gets info of "INBOX"
+    When SMTP client "s2" sends "HELO example.com"
+    Then IMAP response to "i2" is "OK"
+    Then SMTP response to "s2" is "OK"
+    When there is no internet connection
+    And 1 second pass
+    Then IMAP client "i2" is logged out
+    And SMTP client "s2" is logged out
+    Given the internet connection is restored
+    And there is IMAP client "i3" logged in as "user"
+    And there is SMTP client "s3" logged in as "user"
+    When IMAP client "i3" gets info of "INBOX"
+    When SMTP client "s3" sends "HELO example.com"
+    Then IMAP response to "i3" is "OK"
+    Then SMTP response to "s3" is "OK"

test/imap_actions_mailbox_test.go

@@ -30,6 +30,7 @@ func IMAPActionsMailboxFeatureContext(s *godog.Suite) {
 	s.Step(`^IMAP client lists mailboxes$`, imapClientListsMailboxes)
 	s.Step(`^IMAP client selects "([^"]*)"$`, imapClientSelects)
 	s.Step(`^IMAP client gets info of "([^"]*)"$`, imapClientGetsInfoOf)
+	s.Step(`^IMAP client "([^"]*)" gets info of "([^"]*)"$`, imapClientNamedGetsInfoOf)
 	s.Step(`^IMAP client gets status of "([^"]*)"$`, imapClientGetsStatusOf)
 }
 
@@ -74,8 +75,12 @@ func imapClientSelects(mailboxName string) error {
 }
 
 func imapClientGetsInfoOf(mailboxName string) error {
-	res := ctx.GetIMAPClient("imap").GetMailboxInfo(mailboxName)
+	return imapClientNamedGetsInfoOf("imap", mailboxName)
-	ctx.SetIMAPLastResponse("imap", res)
+}
+
+func imapClientNamedGetsInfoOf(clientName, mailboxName string) error {
+	res := ctx.GetIMAPClient(clientName).GetMailboxInfo(mailboxName)
+	ctx.SetIMAPLastResponse(clientName, res)
 	return nil
 }
 

test/imap_checks_test.go

@@ -40,6 +40,8 @@ func IMAPChecksFeatureContext(s *godog.Suite) {
 	s.Step(`^IMAP client receives update marking message seq "([^"]*)" as unread within (\d+) seconds$`, imapClientReceivesUpdateMarkingMessageSeqAsUnreadWithin)
 	s.Step(`^IMAP client "([^"]*)" receives update marking message seq "([^"]*)" as unread within (\d+) seconds$`, imapClientNamedReceivesUpdateMarkingMessageSeqAsUnreadWithin)
 	s.Step(`^IMAP client "([^"]*)" does not receive update for message seq "([^"]*)" within (\d+) seconds$`, imapClientDoesNotReceiveUpdateForMessageSeqWithin)
+	s.Step(`^IMAP client is logged out$`, imapClientIsLoggedOut)
+	s.Step(`^IMAP client "([^"]*)" is logged out$`, imapClientNamedIsLoggedOut)
 }
 
 func imapResponseIs(expectedResponse string) error {
@@ -136,3 +138,13 @@ func iterateOverSeqSet(seqSet string, callback func(string)) {
 		}
 	}
 }
+
+func imapClientIsLoggedOut() error {
+	return imapClientNamedIsLoggedOut("imap")
+}
+
+func imapClientNamedIsLoggedOut(clientName string) error {
+	res := ctx.GetIMAPClient(clientName).SendCommand("CAPABILITY")
+	res.AssertError("read response failed:")
+	return ctx.GetTestingError()
+}

test/mocks/imap_client.go

@@ -43,6 +43,9 @@ type IMAPClient struct {
 func NewIMAPClient(t TestingT, tag string, imapAddr string) *IMAPClient {
 	conn, err := net.Dial("tcp", imapAddr)
 	require.NoError(t, err)
+	if err != nil {
+		return &IMAPClient{}
+	}
 	response := bufio.NewReader(conn)
 
 	// Read first response to opening connection.

test/mocks/imap_response.go

@@ -117,7 +117,7 @@ func (ir *IMAPResponse) AssertResult(wantResult string) *IMAPResponse {
 func (ir *IMAPResponse) AssertError(wantErrMsg string) *IMAPResponse {
 	ir.wait()
 	if ir.err == nil {
-		a.Fail(ir.t, "Expected error %s", wantErrMsg)
+		a.Fail(ir.t, "Error is nil", "Expected to have %q", wantErrMsg)
 	} else {
 		a.Regexp(ir.t, wantErrMsg, ir.err.Error(), "Expected error %s but got %s", wantErrMsg, ir.err)
 	}

test/mocks/smtp.go

@@ -30,6 +30,7 @@ import (
 
 	"github.com/ProtonMail/go-rfc5322"
 	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -46,6 +47,12 @@ type SMTPClient struct {
 func NewSMTPClient(t TestingT, tag, smtpAddr string) *SMTPClient {
 	conn, err := net.Dial("tcp", smtpAddr)
 	require.NoError(t, err)
+	if err != nil {
+		return &SMTPClient{}
+	}
+	logrus.WithField("addr", conn.LocalAddr().String()).
+		WithField("tag", tag).
+		Debug("SMTP Dialed")
 	response := bufio.NewReader(conn)
 
 	// Read first response to opening connection.
@@ -85,6 +92,7 @@ func (c *SMTPClient) SendCommands(commands ...string) *SMTPResponse {
 		message, err := c.response.ReadString('\n')
 		if err != nil {
 			smtpResponse.err = fmt.Errorf("read response failed: %v", err)
+			c.debug.printErr(smtpResponse.err.Error() + "\n")
 			return smtpResponse
 		}
 

test/mocks/smtp_response.go

@@ -34,7 +34,7 @@ func (sr *SMTPResponse) AssertOK() *SMTPResponse {
 
 func (sr *SMTPResponse) AssertError(wantErrMsg string) *SMTPResponse {
 	if sr.err == nil {
-		a.Fail(sr.t, "Expected error %s", wantErrMsg)
+		a.Fail(sr.t, "Error is nil", "Expected to have %q", wantErrMsg)
 	} else {
 		a.Regexp(sr.t, wantErrMsg, sr.err.Error(), "Expected error %s but got %s", wantErrMsg, sr.err)
 	}

test/smtp_actions_test.go

@@ -37,6 +37,7 @@ func SMTPActionsAuthFeatureContext(s *godog.Suite) {
 	s.Step(`^SMTP client sends message with bcc "([^"]*)"$`, smtpClientSendsMessageWithBCC)
 	s.Step(`^SMTP client "([^"]*)" sends message with bcc "([^"]*)"$`, smtpClientNamedSendsMessageWithBCC)
 	s.Step(`^SMTP client sends "([^"]*)"$`, smtpClientSendsCommand)
+	s.Step(`^SMTP client "([^"]*)" sends "([^"]*)"$`, smtpClientNamedSendsCommand)
 }
 
 func smtpClientAuthenticates(bddUserID string) error {
@@ -108,9 +109,12 @@ func smtpClientNamedSendsMessageWithBCC(clientID, bcc string, message *gherkin.D
 }
 
 func smtpClientSendsCommand(command string) error {
+	return smtpClientNamedSendsCommand("smtp", command)
+}
+func smtpClientNamedSendsCommand(clientName, command string) error {
 	command = strings.ReplaceAll(command, "\\r", "\r")
 	command = strings.ReplaceAll(command, "\\n", "\n")
-	res := ctx.GetSMTPClient("smtp").SendCommands(command)
+	res := ctx.GetSMTPClient(clientName).SendCommands(command)
-	ctx.SetSMTPLastResponse("smtp", res)
+	ctx.SetSMTPLastResponse(clientName, res)
 	return nil
 }

test/smtp_checks_test.go

@@ -24,6 +24,8 @@ import (
 func SMTPChecksFeatureContext(s *godog.Suite) {
 	s.Step(`^SMTP response is "([^"]*)"$`, smtpResponseIs)
 	s.Step(`^SMTP response to "([^"]*)" is "([^"]*)"$`, smtpResponseNamedIs)
+	s.Step(`^SMTP client is logged out`, smtpClientIsLoggedOut)
+	s.Step(`^SMTP client "([^"]*)" is logged out`, smtpClientNamedIsLoggedOut)
 }
 
 func smtpResponseIs(expectedResponse string) error {
@@ -39,3 +41,13 @@ func smtpResponseNamedIs(clientID, expectedResponse string) error {
 	}
 	return ctx.GetTestingError()
 }
+
+func smtpClientIsLoggedOut() error {
+	return smtpClientNamedIsLoggedOut("smtp")
+}
+
+func smtpClientNamedIsLoggedOut(clientName string) error {
+	res := ctx.GetSMTPClient(clientName).SendCommands("HELO loggedOut.com")
+	res.AssertError("read response failed:")
+	return ctx.GetTestingError()
+}