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

Support for OTP 27 process labels #442

Merged
merged 16 commits into from
Jun 13, 2024
Merged

Conversation

hugobarauna
Copy link
Member

Before example

CleanShot 2024-06-11 at 17 44 03

After example

CleanShot 2024-06-11 at 17 44 34

URL with a notebook showing the feature being used in multiple contexts: https://gist.github.com/hugobarauna/6dd8b05b51d66bf313c3870c2bc428ed.

You can import that notebook, with Livebook running with Elixir 1.17 rc and see the diagrams with process labels.

@hugobarauna hugobarauna changed the title Support for OTP27 process labels Support for OTP 27 process labels Jun 11, 2024
lib/kino/process.ex Outdated Show resolved Hide resolved
@@ -315,6 +316,9 @@ defmodule Kino.Process do

def seq_trace(trace_pids, trace_function, opts)
when is_list(trace_pids) or trace_pids == :all do
# Set up the tracer responsible for storing calls to Process.set_label/1
{:ok, process_label_tracer_pid} = LabelTracer.start_link()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a process?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was my second approach, the first didn't work.

In the first one, I was trying to get the process label from the process registry directly, using :proc_lib.get_label/1. But the problem was that by the time I was calling that function, the process I was trying to access the label from could have already died.

That happened in that simple example:

parent = self()

Kino.Process.render_seq_trace(fn ->
  child =
    spawn(fn ->
      Process.set_label(:ponger)

      receive do
        :ping ->
          send(parent, :pong)
      end
    end)

  send(child, :ping)

  receive do
    :pong -> :ponged!
  end
end)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's expected, you will always have this race condition. It also happens for names. We only take a snapshot. We should avoid the tracer in my opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the "ping <-> pong" example, it's not a race condition; it's deterministic

In this example, we won't be able to access the :ponger label using :proc_lib.get_label/1 because we first run the tracer function (the anonymous function passed to Kino.Process.render_seq_trace), and only after that do we'd try to access the labels of the processes inside. By that time, the :ponger process is already dead (deterministically).

Since the "ping <-> pong" example is our (theoretically) "canonical example" for showing the sequence diagrams for rendering message passing, I'd prefer if our solution would work for showing process labels for that example.

What do you think?

Maybe I'm not seeing the downside of using the tracer here, since it's the first time I'm using it.

One downside I can see in the current implementation is that if the developer sets the process label using :proc_lib.set_label/1 (from Erlang) instead of using the Elixir wrapper (Process.set_label/1), then the current code wouldn't get the process label. But we could easily address that by tracing that function call as well, if we want to."

Copy link
Member

Choose a reason for hiding this comment

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

Btw. we already have a tracker process, we could at least reuse it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Btw. we already have a tracker process, we could at least reuse it?

That may be possible. I can investigate that path.

Copy link
Member Author

Choose a reason for hiding this comment

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

@josevalim ok, it should be fixed now.

Still there's a race condition, but I think it's ok.

For example, with the new solution, I was able to reproduce the race condition with that example:

parent = self()

child =
  spawn(fn ->
    Process.set_label(:ponger)

    receive do
      :ping ->
       # theres's a race condition here, without that sleep
       # when the tracer tries to get the process label
       # the ponger process is already dead
       Process.sleep(1) 
       send(parent, :pong)
    end
  end)

Kino.Process.render_seq_trace(fn ->
  send(child, :ping)

  receive do
    :pong -> :ponged!
  end
end)

Also, the old solution had a bug, as you mentioned, it could not get process labels set outside the traced function. Now that's working:

CleanShot 2024-06-13 at 10 53 02

lib/kino/process.ex Outdated Show resolved Hide resolved
lib/kino/process.ex Outdated Show resolved Hide resolved
lib/kino/process.ex Outdated Show resolved Hide resolved
lib/kino/process.ex Outdated Show resolved Hide resolved
lib/kino/process.ex Outdated Show resolved Hide resolved
hugobarauna and others added 2 commits June 13, 2024 10:38
Co-authored-by: Jonatan Kłosko <[email protected]>
Co-authored-by: Jonatan Kłosko <[email protected]>
@hugobarauna
Copy link
Member Author

@josevalim @jonatanklosko, I think this should be good to go. Can you please take a last look?

lib/kino/process.ex Outdated Show resolved Hide resolved
Copy link
Contributor

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

One comment and we can ship it!!!!

@hugobarauna hugobarauna merged commit e587af3 into main Jun 13, 2024
1 check passed
@hugobarauna hugobarauna deleted the hb-support-process-labels branch June 13, 2024 16:04
Comment on lines +450 to +453
pid_text = :erlang.pid_to_list(pid) |> List.to_string()

label = process_label |> inspect() |> String.replace(~s{"}, "")
"#{label}<br/>#{pid_text}"
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick, I'd change to:

    label = process_label |> inspect() |> String.replace(~s{"}, "")
    "#{label}<br/>#{:erlang.pid_to_list(pid)}"

I tried pushing to main but I don't have access. :D No big deal anyway of course!

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.

4 participants