Page MenuHomePhabricator

RFC: Clarify / clean up return values from ServiceRunner.run()
Closed, ResolvedPublic

Description

Currently, ServiceWorker.run()'s return value differs depending on whether num_workers is 0 or not. Without forking (num_workers = 0), an array with the actual module return values is returned. With forking, nothing is returned.

To resolve this inconsistency, we will need to more clearly define which kinds of objects can be returned. In the fork case, we need to be able to serialize return values. This means that we'll need to restrict return values to JSON.stringify-able values.

Event Timeline

Restricted Application added subscribers: Zppix, Aklapper. · View Herald TranscriptMay 18 2016, 8:21 PM
Danny_B renamed this task from RFC: Clarify / clean up return values from ServiceRunner.run() to Clarify / clean up return values from ServiceRunner.run().May 18 2016, 8:41 PM
Danny_B added a project: Proposal.
GWicke renamed this task from Clarify / clean up return values from ServiceRunner.run() to RFC: Clarify / clean up return values from ServiceRunner.run().EditedMay 18 2016, 11:17 PM
GWicke added a subscriber: Danny_B.

@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.

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?

GWicke added a comment.EditedMay 19 2016, 3:14 PM

@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?

GWicke added a comment.EditedMay 19 2016, 7:48 PM

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.
Arlolra added a comment.EditedMay 19 2016, 8:19 PM

That sounds good too :)

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

ssastry moved this task from Backlog to Non-Parsoid Tasks on the Parsoid board.May 23 2016, 3:03 PM
GWicke closed this task as Resolved.Jun 14 2016, 6:17 PM

This is now published as 2.0.