fix(server): fix leakage of SessionMetaData

[tw4452852]

Currently, when the server exits, Session data is leaked because 'server_listener' thread still holds a reference to it.
Due to this thread blocking on the socket accepting, to fix this problem, we change it to polling mode (the timer interval is 10 milliseconds) and add a flag to indicate exit.

Signed-off-by: Tw tw19881113@gmail.com

[kunalmohan]

@tw4452852 Thanks for catching this! I'm a bit curious about how big of an issue this is and whether there is an alternate approach to solve this. This only happens when zellij is exiting and IIUC the OS releases any memory allocated to a process after it exits.

The reason I'm concerned is that the new change adds polling to the thread so the thread will wake up every 10ms. We had a similar polling approach in some other section of zellij in some of the earlier releases due to which users complained of higher CPU and power consumption (Linked Issue: #509).

[tw4452852]

Hi @kunalmohan

Indeed, any allocated memory will be released by the OS when the process exits. But as I did some temporary files cleanup in PR #723 when wasm thread exits. And this depends on the notification sent when SessionMetaData drops:

zellij/zellij-server/src/lib.rs

Line 106 in 72b0474

let _ = self.senders.send_to_plugin(PluginInstruction::Exit);

Also if we add more actions when threads exit in the future, this really needs to be fixed I think.

Regarding the CPU usage when polling, it's almost ignorable compared to the blocking mode. As I use this during my daily work.

[imsnif]

I hope it's okay if I chime in here: if this is in order to facilitate something else that hasn't been merged yet, maybe we can look into that first and then see if we still need this one? I guess @TheLostLambda will probably have some useful input regarding cached plugin disk metadata.

[kunalmohan]

FWIW we drop the SessionMetaData object on exit here:

zellij/zellij-server/src/lib.rs

Line 270 in 72b0474

*session_data.write().unwrap() = None;

But there are other cases where we don't drop it on exit (this and this). So maybe we can move the above statement outside the loop, before:

zellij/zellij-server/src/lib.rs

Lines 300 to 304 in 72b0474

	thread_handles
	.lock()
	.unwrap()
	.drain(..)
	.for_each(|h| drop(h.join()));

[tw4452852]

Ah, I see!

[tw4452852]

@kunalmohan
BTW, after re-reading the code, I think the second clone is unnecessary:

zellij/zellij-server/src/lib.rs

Lines 167 to 187 in 72b0474

	let session_data = session_data.clone();
	let session_state = session_state.clone();
	let to_server = to_server.clone();
	thread_handles.lock().unwrap().push(
	thread::Builder::new()
	.name("server_router".to_string())
	.spawn({
	let session_data = session_data.clone();
	let os_input = os_input.clone();
	let to_server = to_server.clone();
	move || {
	route_thread_main(
	session_data,
	session_state,
	os_input,
	to_server,
	)
	}
	})
	.unwrap(),

Correct me if I'm wrong.

[kunalmohan]

@tw4452852 yes, you are right!

[tw4452852]

Thanks @kunalmohan

I have updated the patch according to your suggestions.

[kunalmohan]

LGTM! Thanks!!