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

ArgumentError in Sibyl.Handlers.attach_all_events/2 in docker #30

Closed
florius0 opened this issue Sep 10, 2023 · 11 comments
Closed

ArgumentError in Sibyl.Handlers.attach_all_events/2 in docker #30

florius0 opened this issue Sep 10, 2023 · 11 comments

Comments

@florius0
Copy link
Contributor

florius0 commented Sep 10, 2023

I was evaluating Sybil and came across this error:

** (Mix) Could not start application fish_finder: exited in: FF.Application.start(:normal, [])
     ** (EXIT) an exception was raised:
         ** (ArgumentError) errors were found at the given arguments:

   * 1st argument: invalid destination

             :erlang.send(:undefined, {:shell_state, #PID<0.3858.0>})
             (stdlib 5.0.1) shell.erl:590: :shell.get_state/0
             (stdlib 5.0.1) shell.erl:597: :shell.get_function/2
             (stdlib 5.0.1) shell_default.erl:159: :shell_default."$handle_undefined_function"/2
             (sibyl 0.1.2) lib/sibyl/events.ex:105: Sibyl.Events.reflect/1
             (elixir 1.15.0) lib/enum.ex:4317: Enum.flat_map_list/2
             (elixir 1.15.0) lib/enum.ex:4318: Enum.flat_map_list/2
             (sibyl 0.1.2) lib/sibyl/handlers.ex:20: Sibyl.Handlers.attach_all_events/2
             (fish_finder 0.1.0) lib/fish_finder/application.ex:29: FF.Application.start/2
             (kernel 9.0.1) application_master.erl:293: :application_master.start_it_old/4

It was not reproduceable on my local elixir, but only in docker.

My repo using which the bug may be reproduced.

From what I'm gathering, :shell.__info__ results not in exception, but in message sending to :undefied pid.

Versions

Local:

  • OS: macOS 13.4
  • Erlang/Elixir:
Erlang/OTP 25 [erts-13.2.2.1] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:1] [jit:ns] [dtrace]

Elixir 1.15.0 (compiled with Erlang/OTP 25)

Docker:

  • Runtime: Orbstack 0.16.1
  • bitwalker/alpine-elixir-phoenix:1.15.0
@vereis
Copy link
Collaborator

vereis commented Sep 11, 2023

Beautiful, thanks for the reproduction steps.

Will take a look and get back to you soon 🤞

@florius0
Copy link
Contributor Author

I might be able to fix it today or narrow the problem at least

@florius0
Copy link
Contributor Author

Using module_info/2 instead of __info__/2 in Sibyl.Events.reflect/1 seems have fixed the issue.

In my fork I've updated to Elixir 1.15.0 (tho I'm not sure yet that it didn't caused any problems), and renamed guards and predicates to match elixir conventions (is_... for guards, ...? for predicates).

If you would like I can include those changes in PR.

PS
I would like to suggest to run your CI on all minor versions from lowest supported up to latest

@vereis
Copy link
Collaborator

vereis commented Sep 13, 2023

@florius0 that sounds awesome! Looking forward to seeing a PR 🙌 Thank you!

And yes, running CI against a matrix of supported versions (defining what those are explicitly) will be great too, thank you for the suggestion!

@florius0
Copy link
Contributor Author

florius0 commented Sep 23, 2023

Tried it on elixir:1.15.4-otp-25-alpine, same issue. Perhaps it has to do smth with alpine. My fix works tho

@rob-vetspire
Copy link

I can confirm issue #30 also occurs when running an application with elixir 1.15.7-otp-26 and erlang 26.1.2 directly on macos.

@florius0
Copy link
Contributor Author

I guess it is related to new stripping of unused functions by elixir compiler maybe...

@vereis
Copy link
Collaborator

vereis commented Oct 24, 2023

@florius0 after investigating, I think we should just skip all Erlang modules when doing reflection, as reflection will only discover events defined by Sibyl, and that can only happen in Elixir applications.

I think this is probably a better fix than relying on module.module_info, as at the very least, the __info__/1 callback is documented: https://hexdocs.pm/elixir/1.7.3/Module.html

Have the tentative fix in #34, will just need to do some regression testing but I believe this fixes the problem!

Apologies for the delay!

@florius0
Copy link
Contributor Author

I don't think that restricting Erlang compatibility is a good option especially when there is no cost of keeping it.

If my memory serves me, Sybil defines events as module attributes (with persist: true), and getting such attribute with module_info/1 is ok.
It is not documented in Elixir docs since it is Erlang callback: https://www.erlang.org/doc/reference_manual/modules#module_info-0-and-module_info-1-functions.

As for delay, do not worry, we use my fork for the time being.

@vereis
Copy link
Collaborator

vereis commented Oct 24, 2023

Ah I wasn't aware of that. Will revisit my change, thanks ❤️

@vereis
Copy link
Collaborator

vereis commented Nov 15, 2023

Resolved in #34

@vereis vereis closed this as completed Nov 15, 2023
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 a pull request may close this issue.

3 participants