Add shared memory file to rediscover sessions #800 #819
Conversation
|
@speidy - I have implemented your suggestions in the two commits above. |
|
Just hit #800 and was looking to see if it has been fixed and found this PR - any chance this could get picked up and worked on again, as this would be really useful to have! |
|
Hey guys. Do you have any plan to include this? |
Allow xrdp-sesman to discover sessions still running that were created by a previous xrdp-sesman process. Implement this using a file-backed shared mmap region, visible to both the daemon and session instances of xrdp-sesman. Add a heartbeat timestamp updated by the session instance so that the daemon instance can infer whether the sessions in the mmap region are current or stale. The shared memory can also be used to pass other data between daemon and session instances, for example the session's idle time. Add locking around access to g_sessions and the mmap region. This is a PTHREAD_PROCESS_SHARED lock which protects shared memory used by separate processes. Defining DEBUG_SESSION_LOCK enables logging to help debug locking bugs. Notes: 1. The number of sessions is limited by the size of the array in shared memory, SESMAN_SHAREDMEM_MAX_SESSIONS. This could be made dynamic instead by growing the file and the mmap region. 2. If sesshm_try_open_existing_shm() finds a shm file but it looks wrong, it creates a new one. Perhaps it should instead exit with an error? 3. In sesshm_thread() if the session xrdp-sesman notices that it has been removed from the daemon's list it exits. It should kill the X server and/or window manager first. 4. This doesn't yet update idle times. I need to work out how to get this information from the X server. 5. This uses an array of sessions in the mmap region so the linked list g_sessions might be redundant now. 6. Defining DONT_USE_SHM will let xrdp-sesman run without creating or trying to use the mmap file. This could be removed in the future. 7. I think the locking fixes an theorised bug where a session can be removed from the g_sessions linked list in session_kill() (called from the signal handler) at the same time as a session is added in session_start_fork().
1. Add brackets round if bodies.
2. Use common os_calls/thread_calls functions where available and add
new ones for:
g_file_seek_rel_end()
g_map_file_shared()
g_unmap_file_shared()
g_sleep_secs() [because usleep() can error for argument >= 1M]
g_ftruncate()
tc_mutex_timed_lock()
tc_shared_mutex_create()
3. Separate declarations and initialisations.
4. Make macros for sesshm_try_lock() etc cleaner.
Also some minor bugfixes:
1. Fix bug in sesshm_try_lock() which begins "return 0".
2. The return value of pthread_mutex_init() in sesshm_create_new_shm()
was ignored. (No longer relevant because tc_shared_mutex_create() is
now used.)
3. sesshm_map() set g_shm_mapping and returned a value. (No longer
relevant because using g_map_file_shared() it is simple enough not to
need a function.)
ben-cohen
force-pushed
the
discover-sessions-devel
branch
from
8158fc6 to
03dbc95
Compare
|
This is of significant interest to people who have things like Debian/Ubuntu unattended-upgrades set up with needrestart. The latter willl restart systemd services impacted by an update, and this includes xrdp-sesman. As a result, after an upgrade of xrdp, I lose the ability to reattach to already-running sessions. It seems merging this change will allow xrdp to "reclaim" the previously running sessions (modulo maintaining backward compatibility with the contents of the file across versions, or at least detecting incompatibility upon load, which is a whole other issue). This would resolve the problem. (Instead of using a memory-mapped file which carries the risk that the structures change across xrdp upgrades, I would suggest instead using a SIGINT/SIGTERM/SIGQUIT handler that serializes out the structures into a forgiving format, e.g. JSON, where a newer version can load what is present and default the rest, and also the object can carry a file format version that a newer version can check against to determine whether it still has the ability to load it.) |
|
Hi all, I'm a little late to this conversation I'm afraid. I absolutely agree this functionality is necessary from a restarting services perspective. At the moment we're working on a rather important fix for a systemd change (#1684), which is looking like it's going to break this PR I'm afraid. It's not all bad news however. Some of the functions of sesman need to be moved into the session leader process which runs the user session. This is going to involve active communication between sesman and the session leader. Once this is done, we can arrange for the session leader to detect when sesman restarts and re-send the state data sesman needs to be able to re-establish connections. I think this is a better systems design. The PR here, whether or not we're using a separate file, can't track session state changes while sesman is down. The session leaders however will be able to do this while sesman is stopped. Please feel free to come back to me with any questions or comments about this. |
|
This should be reconsidered after #1961. |
Allow xrdp-sesman to discover sessions still running that were created
by a previous xrdp-sesman process.
Implement this using a file-backed shared mmap region, visible to both
the daemon and session instances of xrdp-sesman. Add a heartbeat
timestamp updated by the session instance so that the daemon instance
can infer whether the sessions in the mmap region are current or stale.
The shared memory can also be used to pass other data between daemon and
session instances, for example the session's idle time.
Add locking around access to g_sessions and the mmap region. This is a
PTHREAD_PROCESS_SHARED lock which protects shared memory used by
separate processes. Defining DEBUG_SESSION_LOCK enables logging to help
debug locking bugs.
Notes:
The number of sessions is limited by the size of the array in shared
memory, SESMAN_SHAREDMEM_MAX_SESSIONS. This could be made dynamic
instead by growing the file and the mmap region.
If sesshm_try_open_existing_shm() finds a shm file but it looks
wrong, it creates a new one. Perhaps it should instead exit with an
error?
In sesshm_thread() if the session xrdp-sesman notices that it has
been removed from the daemon's list it exits. It should kill the X
server and/or window manager first.
This doesn't yet update idle times. I need to work out how to get
this information from the X server.
This uses an array of sessions in the mmap region so the linked list
g_sessions might be redundant now.
Defining DONT_USE_SHM will let xrdp-sesman run without creating or
trying to use the mmap file. This could be removed in the future.
I think the locking fixes an theorised bug where a session can be
removed from the g_sessions linked list in session_kill() (called from
the signal handler) at the same time as a session is added in
session_start_fork().