Skip to content

Add GenServer.format_status/1 callback #13511

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

Merged
merged 6 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions lib/elixir/lib/dynamic_supervisor.ex
Original file line number Diff line number Diff line change
Expand Up @@ -1136,15 +1136,6 @@ defmodule DynamicSupervisor do
]
end

@impl true
def format_status(:terminate, [_pdict, state]) do
state
end

def format_status(_, [_pdict, %{mod: mod} = state]) do
[data: [{~c"State", state}], supervisor: [{~c"Callback", mod}]]
Copy link
Contributor Author

@sabiwara sabiwara Apr 23, 2024

Choose a reason for hiding this comment

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

I don't think we can reproduce this behavior with format_status/1, and this breaks a test - should I just keep this one around for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we can just remove these tests, or implement them to use :mod?

If :supervisor.get_callback_module is used somewhere which matters, should erlang add a key to the format_status map?

Copy link
Member

Choose a reason for hiding this comment

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

@sabiwara we should mirror whatever the Erlang supervisor is doing. This was most likely just borrowed from them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK nice, seems they just removed it.
erlang/otp@67513dc#diff-030fc115adbcd287aecbd623b8355c9cf9aa1fd05ef07b4001b05ce927ec5e03

OK to remove the tests completely then?

Copy link
Member

Choose a reason for hiding this comment

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

Yes!

end

## Helpers

@compile {:inline, call: 2}
Expand Down
2 changes: 2 additions & 0 deletions lib/elixir/lib/gen_event.ex
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,8 @@ defmodule GenEvent do
{:ok, states, [name, handlers, hib]}
end

# Keeping deprecated format_status/2 since the current implementation is not
# compatible with format_status/1 and GenEvent is deprecated anyway
@doc false
def format_status(opt, status_data) do
[pdict, sys_state, parent, _debug, [name, handlers, _hib]] = status_data
Expand Down
39 changes: 28 additions & 11 deletions lib/elixir/lib/gen_server.ex
Original file line number Diff line number Diff line change
Expand Up @@ -759,22 +759,38 @@ defmodule GenServer do
when old_vsn: term | {:down, term}

@doc """
Invoked in some cases to retrieve a formatted version of the `GenServer` status:
This function is called by a `GenServer` process in the following situations:

* one of `:sys.get_status/1` or `:sys.get_status/2` is invoked to get the
status of the `GenServer`; in such cases, `reason` is `:normal`
* [`:sys.get_status/1,2`](`:sys.get_status/1`) is invoked to get the `GenServer` status.
* The `GenServer` process terminates abnormally and logs an error.

* the `GenServer` terminates abnormally and logs an error; in such cases,
`reason` is `:terminate`
This callback is used to limit the status of the process returned by
[`:sys.get_status/1,2`](`:sys.get_status/1`) or sent to logger.

This callback can be useful to control the *appearance* of the status of the
`GenServer`. For example, it can be used to return a compact representation of
the `GenServer`'s state to avoid having large state terms printed.
The callback gets a map `status` describing the current status and shall return
a map `new_status` with the same keys, but it may transform some values.

Two possible use cases for this callback is to remove sensitive information
from the state to prevent it from being printed in log files, or to compact
large irrelevant status items that would only clutter the logs.

## Example

@impl GenServer
def format_status(status) do
Map.new(status, fn
{:state, state} -> {:state, Map.delete(state, :private_key)}
{:message, {:password, _}} -> {:message, {:password, "redacted"}}
key_value -> key_value
end)
end

`pdict_and_state` is a two-elements list `[pdict, state]` where `pdict` is a
list of `{key, value}` tuples representing the current process dictionary of
the `GenServer` and `state` is the current state of the `GenServer`.
"""
@doc since: "1.17.0"
@callback format_status(status :: :gen_server.format_status()) ::
new_status :: :gen_server.format_status()

@doc deprecated: "Use format_status/1 callback instead"
@callback format_status(reason, pdict_and_state :: list) :: term
when reason: :normal | :terminate
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should still document it. In any case, let's also add a TODO to remove it on 2.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, let's remove the docs completely.

Copy link
Member

Choose a reason for hiding this comment

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

Let's add the TODO and ship it!


Expand All @@ -783,6 +799,7 @@ defmodule GenServer do
handle_info: 2,
handle_cast: 2,
handle_call: 3,
format_status: 1,
format_status: 2,
handle_continue: 2

Expand Down
2 changes: 1 addition & 1 deletion lib/elixir/pages/references/typespecs.md
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ Optional callbacks can be defined through the `@optional_callbacks` module attri
@optional_callbacks non_vital_fun: 0, non_vital_macro: 1
end

One example of optional callback in Elixir's standard library is `c:GenServer.format_status/2`.
One example of optional callback in Elixir's standard library is `c:GenServer.format_status/1`.

### Inspecting behaviours

Expand Down
8 changes: 0 additions & 8 deletions lib/elixir/test/elixir/dynamic_supervisor_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -140,14 +140,6 @@ defmodule DynamicSupervisorTest do
{:ok, pid} = DynamicSupervisor.start_link(strategy: :one_for_one)
assert :proc_lib.initial_call(pid) == {:supervisor, Supervisor.Default, [:Argument__1]}
end

test "returns the callback module" do
{:ok, pid} = Supervisor.start_link([], strategy: :one_for_one)
assert :supervisor.get_callback_module(pid) == Supervisor.Default

{:ok, pid} = DynamicSupervisor.start_link(strategy: :one_for_one)
assert :supervisor.get_callback_module(pid) == Supervisor.Default
end
end

## Code change
Expand Down
Loading