Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Add kindaUnlift to find out what happens where #43

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dpwiz
Copy link

@dpwiz dpwiz commented Apr 10, 2023

Works like this...

server :: Troupe.Process NodeEnv ()
server = do
  serverPids <- (,) <$> myThreadId <*> Troupe.self
  traceShowM ("Server" :: Text, serverPids)

  Troupe.kindaUnlift \unlift ->
    liftIO $ Server.run config \connection ->
      -- The server does `forkIO` for each incoming connection
      unlift Troupe.Unbound do
        -- Unlift spawns a __new__ Process that has access to connection and server's process context
        handlerPids <- (,) <$> myThreadId <*> Troupe.self
        traceShowM ("Handler" :: Text, handlerPids)

Copy link
Owner

@NicolasT NicolasT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to put this in Request changes mode for now, since I think some things need to be figured out before we move forward.

As an example, imagine there's a supervisor implementation, how could a child Process spawned by Server.run (through an unlift) be added to it? Should it be?... There's a bunch of questions related to ownership and lifetime.

kindaUnlift foreignSpawner = do
env <- getProcessEnv
foreignSpawner $ \affinity action -> do
_pid <- spawnImplWith env affinity pure action -- XXX: does it make sense to link/monitor a wrapped process?
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe so. In the current example (Server.run), unlift is used in an infinite loop, so there it may not make sense. However, there might be cases where it's used one-shot, and hence the parent Process may very well be interested in the child exiting (through monitor), or getting linked.

In a way, I think link should almost be the default: with the current approach, it's way too easy to end up with ghost thread/processes, no?

Copy link
Author

@dpwiz dpwiz Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A server process may wish to monitor spawned handler processes, but no new implementation details would be needed for that.

server = do
  serverPid <- self
  kindaUnlift \unlift -> liftIO do
    config <- Server.setup
    Server.run config \connection ->
      unlift Unbound do
        handlerPid <- self
        -- The handler can now use serverPid and handlerPid to establish monitoring, linking or else
        forever $ doHandlerThings connection
    -- XXX: the server Process is now blocked by running Server loop

env <- getProcessEnv
foreignSpawner $ \affinity action -> do
_pid <- spawnImplWith env affinity pure action -- XXX: does it make sense to link/monitor a wrapped process?
-- TODO: spawnImplWith should have a blocking version
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. Would it make sense for the caller thread to become the monitor of the process, instead of spawning two more threads (one monitor, one to run the effective Process), and hence block?

Not even sure that's the right approach. In a way, I'm bit uncomfortable with how threads would be managed now. Does your Server.run ever kill a thread it spawns? (In which case it'd be this forever one, and we leak 2 child threads). Does Server.run somehow wrap the inner thing and close the connection when it returns?...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does your Server.run ever kill a thread it spawns?

In general, we don't know.

A typical server loop is

run handler =
  bracket open close \socket ->
    forever do
      connection <- accept socket
      void $! forkIO (handler connection)
  • A server doesn't care if a handler thread crashes.
  • A handler may crash when the server quits - when doing things with its connection derived from the now-dead socket.
  • A handler spawning new threads doesn't affect the server and its loop.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I was thinking: what if we invert things a bit? Basically, instead of spawning a monitor thread (as done in spawnImpl), and another one to run the Process code, can we implement things such that unlift spawns a monitor thread, and then re-uses the thread from which it was invoked (i.e., the thread spawned by Server.run) to actually run the Process code?

This way, whatever Server.run does to the thread it spawned (monitor it, send it some async exception,...) will be "just fine": the code running in the thread (now in a Process context) likely already expects to run in a Server.run-spawned thread (and whatever this may mean), and the monitor thread can still ensure monitors/links are notified on the Troupe side of things.

There's one (somewhat major) caveat: we must make sure a single thread is never used for two ProcessContexts, so Server.run really must forkIO, and kinadUnlift can't be used to create an unlift which is then used in some existing Process thread.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this could be done, spawnImpl could be (re)written to basically use the very same pattern when using 'regular' spawn*, kinda inverting the current implementation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, instead of spawning a monitor thread (as done in spawnImpl), and another one to run the Process code, can we implement things such that unlift spawns a monitor thread, and then re-uses the thread from which it was invoked (i.e., the thread spawned by Server.run) to actually run the Process code?

Yes, this is how I see it too.

@dpwiz
Copy link
Author

dpwiz commented Apr 14, 2023

I now think there are two unrelated issues:

  1. Wrapping a handler thread (like in this PR).
  2. Returning to your own process after being liftIOd (exactly like the "non-working" example in Registering "foreign" threads #42).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants