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.

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

Close attach websocket connection on exec exit. Add pong handler. #96

Merged
merged 1 commit into from
May 9, 2020

Conversation

AndrienkoAleksandr
Copy link
Contributor

Close attach websocket connection on exec exit. Add pong handler. Stop activity tracker on exec exit.

Referenced issue:

eclipse-che/che#16601

Signed-off-by: Oleksandr Andriienko [email protected]

@AndrienkoAleksandr AndrienkoAleksandr requested review from mmorhun and tolusha and removed request for evidolob and vparfonov April 29, 2020 20:21
@tolusha tolusha requested a review from sleshchenko April 30, 2020 05:53
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

Already addressed

Does not WriteDataToWsConnections lead to deadlock since there are two locks set - in the method itself + in the called handler.removeConnection

// Write data to the all connections managed by handler.
func (handler *ConnectionHandlerImpl) WriteDataToWsConnections(data []byte) {
	defer handler.wsConnsLock.Unlock()
	handler.wsConnsLock.Lock()

	for _, wsConn := range handler.wsConns {
		if err := wsConn.WriteMessage(websocket.TextMessage, data); err != nil {
			if IsNormalWSError(err) {
				logrus.Errorf("failed to write to ws-conn message. Cause: %v", err)
			}
			handler.removeConnection(wsConn)
		}
	}
}

// Remove connection form handler.
func (handler *ConnectionHandlerImpl) removeConnection(wsConn *websocket.Conn) {
	defer handler.wsConnsLock.Unlock()
	handler.wsConnsLock.Lock()

	for index, wsConnElem := range handler.wsConns {
		if wsConnElem == wsConn {
			handler.wsConns = append(handler.wsConns[:index], handler.wsConns[index+1:]...)
		}
	}
}

An example I used to test

type TestArrayHandler struct {
	array     []int
	wsConnsLock *sync.Mutex
}

func main() {
	test := TestArrayHandler{
		array: []int{1, 2, 3, 4, 5, 6, 7, 8, 9, 10},
		wsConnsLock: &sync.Mutex{},
	}
	defer test.wsConnsLock.Unlock()
	test.wsConnsLock.Lock()
	removeFirst(&test)
}

func removeFirst(t *TestArrayHandler) {
	defer t.wsConnsLock.Unlock()
	t.wsConnsLock.Lock()

	t.array = append(t.array[:0], t.array[1:]...)
}

=>

fatal error: all goroutines are asleep - deadlock!

exec/kubernetes_exec_manager.go Show resolved Hide resolved
ws-conn/ws-connection-handler.go Outdated Show resolved Hide resolved
ws-conn/ws-connection-handler.go Outdated Show resolved Hide resolved
ws-conn/ws-connection-handler.go Show resolved Hide resolved
ws-conn/ws-connection-handler.go Outdated Show resolved Hide resolved
ws-conn/ws-connection-handler.go Outdated Show resolved Hide resolved
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

WriteDataToWsConnections seems still has issue with deadlock, despite it's not really related to this PR I believe it's a critical part which must be addressed ASAP

exec/activity-connection-saver.go Outdated Show resolved Hide resolved
ws-conn/ws-connection-handler.go Outdated Show resolved Hide resolved
ws-conn/ws-connection-handler.go Outdated Show resolved Hide resolved
ws-conn/ws-connection-handler.go Show resolved Hide resolved
@AndrienkoAleksandr
Copy link
Contributor Author

I think deadlock fixed.

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

Good job! 👍

…p activity tracker on exec exit.

Signed-off-by: Oleksandr Andriienko <[email protected]>
@AndrienkoAleksandr AndrienkoAleksandr merged commit abf3dd7 into master May 9, 2020
@AndrienkoAleksandr AndrienkoAleksandr deleted the fixStopTerminals branch May 9, 2020 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants