@Danny_B, this is an RFC in the general sense, as in Request For Comment. We often add RFC in task titles to make it clear that input is desired. Compare: WIP.

One use case that will be harder to support consistently with JSON.stringify-able return values is stopping / shutting down of services. As an example, the hyperswitch tests currently restart services by calling server.close() on the returned service instance. Of course, this is quite hacky & would not work the same way with JSON.stringify-able return values.

Instead, we could expand the API with a stop() method. In a master / worker setup (num_workers >= 1), this could basically shut down all workers, thereby implicitly closing the sockets used by the workers. While a lot cleaner overall than the server.close() hack used in hyperswitch, I'm not sure if we can make this solution work consistently for num_workers = 0. Any ideas for this issue would be much appreciated.

In T135679#2307534, @GWicke wrote:

Instead, we could expand the API with a stop() method. In a master / worker setup (num_workers >= 1), this could basically shut down all workers, thereby implicitly closing the sockets used by the workers. While a lot cleaner overall than the server.close() hack used in hyperswitch, I'm not sure if we can make this solution work consistently for num_workers = 0. Any ideas for this issue would be much appreciated.

I think we can do it, and it should be pretty easy. Basically, just return an object with a stop() method. In it's closure it will have access to the master/worker object and will be able to gracefully shutdown the service.

What it the reason to want JSON-stringifyable return value?

@Pchelolo, with num_workers > 0, anything passed from workers to the master needs to be serialized to JSON. There is no way to share things like server instances. The issue is about finding an interface that will work no matter what the number of workers is, but still allows us to

• pass information like ports up to the caller, and
• supports shutting down the service cleanly.

A stop method would be nice because we're now in the unfortunate position where Parsoid's mocha tests setup several instances w/o tearing them down, and then one's heartbeat messages get received by the others, and the whole thing crashes.

I'm not sure if we can make this solution work consistently for num_workers = 0

Then throw.

Then throw.

You mean, just throw a "not supported" exception when stop() is called with num_workers = 0?

Yup.

Another option would be to change the module interface a bit:

Optionally, each service can return an object containing a close function. This will be filtered from the return value (JSON.stringify drops it), so outside services won't be able to call this directly. However, the worker will keep a reference to the original object. During worker shutdown, an attempt is made to call this function, with a timeout to limit the delay.

Naming options: close matches what is returned from http servers (incl. hyperswitch & service-template), while stop would match the outer interface.

Pros:

• Services can cleanly shut down.
• Works with or without forking.
• When using close(), this works already with hyperswitch and service-template services.

Cons:

• Slightly more complex module / service interface.

That sounds good too :)

Now implemented in https://github.com/wikimedia/service-runner/pull/99.

In T135679#2310301, @GWicke wrote:

Optionally, each service can return an object containing a close function. This will be filtered from the return value (JSON.stringify drops it), so outside services won't be able to call this directly. However, the worker will keep a reference to the original object. During worker shutdown, an attempt is made to call this function, with a timeout to limit the delay.

Naming options: close matches what is returned from http servers (incl. hyperswitch & service-template), while stop would match the outer interface.

I like this, but as I noted on the PR, this brakes compatibility with the tests in service-template-node and its derivatives, so I think we should release as v2.0.0