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

Fix bug in top_process.ex when complex args are in use #389

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Cgboal
Copy link

@Cgboal Cgboal commented Aug 15, 2022

Hi,

I ran into a bug when spawning supervised tasks via Task.async_stream_nolink/6. When spawning tasks with MFA, the TopProcess GenServer will crash when trying to parse the spawned Tasks info if the args passed to it cannot be simply converted to a string during interpolation.

In my case, this occured when passing [[String.t()], map()], which resulted in the following error:

(ArgumentError) cannot convert the given list to a string.

To be converted to a string, a list must either be empty or only
contain the following elements:

  * strings
  * integers representing Unicode code points
  * a list containing one of these three elements

Please check the given list or call inspect/1 to get the list representation, got:

This fix simply adds an inspect call within the string interpolation which prevents this issue. I have tested this and it resolves the issue.

@CLAassistant
Copy link

CLAassistant commented Aug 15, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ edds
❌ Cgboal
You have signed the CLA already but the status is still pending? Let us recheck it.

edds and others added 2 commits August 18, 2022 17:25
We're seeing http connection timeouts due to slow server responses. This
allows users to define custom configuration for the httpc request
options so they can pass in a timeout that is relevant for their servers.
Allow custom httpc request options to be set
@@ -76,7 +76,7 @@ defmodule NewRelic.Sampler.TopProcess do
defp parse(:name, pid, []) do
with {:dictionary, dictionary} <- Process.info(pid, :dictionary),
{m, f, a} <- Keyword.get(dictionary, :"$initial_call") do
"#{inspect(m)}.#{f}/#{a}"
"#{inspect(m)}.#{f}/#{inspect(a)}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something else must be going wrong here. The a in $initial_call is set by Task and is the arity - it is always a number.

https://github.com/elixir-lang/elixir/blob/fb729784e5504f499b0179c4f154e73ea2a6f216/lib/elixir/lib/task/supervised.ex#L77-L85

Are you doing something manually with $initial_call?

Copy link
Author

@Cgboal Cgboal Aug 26, 2022

Choose a reason for hiding this comment

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

Nope not doing anything manually with initial call:

    task_stream =
      Task.Supervisor.async_stream_nolink(
        XXXX.TaskSupervisor,
        batches,
        callback_module,
        :execute,
        [refined_context],
        max_concurrency: callback_module.max_concurrency(),
        timeout: :infinity
      )

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