Browse code

Close logger only after StartLogger call

Signed-off-by: Jim Minter <jminter@redhat.com>

Jim Minter authored on 2017/04/20 19:17:06
Showing 2 changed files
... ...
@@ -102,15 +102,23 @@ func (daemon *Daemon) ContainerAttachRaw(prefixOrName string, stdin io.ReadClose
102 102
 
103 103
 func (daemon *Daemon) containerAttach(c *container.Container, cfg *stream.AttachConfig, logs, doStream bool) error {
104 104
 	if logs {
105
-		logDriver, err := daemon.getLogger(c)
105
+		logDriver, logCreated, err := daemon.getLogger(c)
106 106
 		if err != nil {
107 107
 			return err
108 108
 		}
109
+		if logCreated {
110
+			defer func() {
111
+				if err = logDriver.Close(); err != nil {
112
+					logrus.Errorf("Error closing logger: %v", err)
113
+				}
114
+			}()
115
+		}
109 116
 		cLog, ok := logDriver.(logger.LogReader)
110 117
 		if !ok {
111 118
 			return logger.ErrReadLogsNotSupported
112 119
 		}
113 120
 		logs := cLog.ReadLogs(logger.ReadConfig{Tail: -1})
121
+		defer logs.Close()
114 122
 
115 123
 	LogLoop:
116 124
 		for {
... ...
@@ -45,10 +45,17 @@ func (daemon *Daemon) ContainerLogs(ctx context.Context, containerName string, c
45 45
 		return nil, logger.ErrReadLogsNotSupported
46 46
 	}
47 47
 
48
-	cLog, err := daemon.getLogger(container)
48
+	cLog, cLogCreated, err := daemon.getLogger(container)
49 49
 	if err != nil {
50 50
 		return nil, err
51 51
 	}
52
+	if cLogCreated {
53
+		defer func() {
54
+			if err = cLog.Close(); err != nil {
55
+				logrus.Errorf("Error closing logger: %v", err)
56
+			}
57
+		}()
58
+	}
52 59
 
53 60
 	logReader, ok := cLog.(logger.LogReader)
54 61
 	if !ok {
... ...
@@ -85,23 +92,8 @@ func (daemon *Daemon) ContainerLogs(ctx context.Context, containerName string, c
85 85
 	messageChan := make(chan *backend.LogMessage, 1)
86 86
 	go func() {
87 87
 		// set up some defers
88
-		defer func() {
89
-			// ok so this function, originally, was placed right after that
90
-			// logger.ReadLogs call above. I THINK that means it sets off the
91
-			// chain of events that results in the logger needing to be closed.
92
-			// i do not know if an error in time parsing above causing an early
93
-			// return will result in leaking the logger. if that is the case,
94
-			// it would also have been a bug in the original code
95
-			logs.Close()
96
-			if cLog != container.LogDriver {
97
-				// Since the logger isn't cached in the container, which
98
-				// occurs if it is running, it must get explicitly closed
99
-				// here to avoid leaking it and any file handles it has.
100
-				if err := cLog.Close(); err != nil {
101
-					logrus.Errorf("Error closing logger: %v", err)
102
-				}
103
-			}
104
-		}()
88
+		defer logs.Close()
89
+
105 90
 		// close the messages channel. closing is the only way to signal above
106 91
 		// that we're doing with logs (other than context cancel i guess).
107 92
 		defer close(messageChan)
... ...
@@ -148,11 +140,17 @@ func (daemon *Daemon) ContainerLogs(ctx context.Context, containerName string, c
148 148
 	return messageChan, nil
149 149
 }
150 150
 
151
-func (daemon *Daemon) getLogger(container *container.Container) (logger.Logger, error) {
152
-	if container.LogDriver != nil && container.IsRunning() {
153
-		return container.LogDriver, nil
151
+func (daemon *Daemon) getLogger(container *container.Container) (l logger.Logger, created bool, err error) {
152
+	container.Lock()
153
+	if container.State.Running {
154
+		l = container.LogDriver
155
+	}
156
+	container.Unlock()
157
+	if l == nil {
158
+		created = true
159
+		l, err = container.StartLogger()
154 160
 	}
155
-	return container.StartLogger()
161
+	return
156 162
 }
157 163
 
158 164
 // mergeLogConfig merges the daemon log config to the container's log config if the container's log driver is not specified.