[NicolasFussberger]

Before: 3-way handshake

   Launch Manager                          App
   ──────────────                          ───
   send_sync_.wait   ◀── send_sync_.post ──
   reply_sync_.post  ── reply_sync_.post ─▶ reply_sync_.wait
   send_sync_.wait   ◀── send_sync_.post ──
   sem_destroy(send_sync_)
   sem_destroy(reply_sync_)

This introduces some potential problems that are unnecessary as the reply_sync_.wait and the second send_sync_.wait use fixed wait times of 1s and 100ms which might be off for a system under heavily load.
This can lead to cases where although the App has reported kRunning (first semaphore) it still fails in the daemon because the 2nd and 3rd semaphores timed out.

Simplified behavior:

  Launch Manager                          App
   ──────────────                          ───
   send_sync_.wait   ◀── send_sync_.post ──
          ⋮
   (app terminated and reaped)
   sem_destroy(send_sync_)

Single semaphore to post kRunning.
Semaphore is destroyed when the process terminated.

[danth]

What was the reason for using multiple messages originally?

[NicolasFussberger]

What was the reason for using multiple messages originally?

AFAIK the reasons were:

• ReportRunning could return an error code to the user, so there needed to be an explicit acknowledgement to the application process that it was received (now its a void method, if it would not work the ready timeout in LM would eventually catch it)
• The third message was to be sure that application is no longer waiting on the semaphore so it served as a trigger to delete the semaphore in shared memory (now its just removed when the process dies)

[github-actions]

Documentation preview for this pull request is available at:
pr-829: https://eclipse-score.github.io/lifecycle/pr-829/

[WilliamRoebuck]

Looks good, nice that we can get rid of the old comms type! I think some of the changes in ipc_comms.hpp make it a bit more confusing than it was before though so I've suggested some changes there. Managing the semaphores with the destructor is a good change, but I found it hard to understand where each one would be called.

[WilliamRoebuck]

Suggested change

	/// The `pid_` is written by the child process right after fork. The client checks it against its own pid.
	/// The `pid_` is used as an expected bit pattern on the client side.
	/// The `pid_` is written by the child process right after fork. The client checks it against its own pid
	/// to verify that the data is valid.

Nitpick but I think The pid_ is used as an expected bit pattern is a bit unhelpful if you haven't seen that part of the code already.

[WilliamRoebuck]

This file seems very confusing now. Is there a way we can implement this without nesting destructors?
If I understand correctly, the deinit and unmap now happen one after another rather than deinit as soon as we are done with the semaphore, so maybe the two destructors can be combined? Alternatively, it might be more readable to return a separate pointer + destructor for the semaphores.

[WilliamRoebuck]

The optional timeout was only used for ignoring kRunning and carrying out the rest of the handshake which we don't need anymore. This is used at process_info_node.cpp:401-408 which can be removed now and then the timeout can be mandatory.