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

Add nitrogen:handler/3 #147

Merged
merged 1 commit into from
Aug 3, 2022
Merged

Conversation

joaohf
Copy link
Contributor

@joaohf joaohf commented Jul 8, 2022

There are cases where the beam modules are stripped and is not possible
to get the attribute lists calling Module:module_info(attributes).

So the caller should pass the Name handler as the first parameter.
Avoiding calling the module_info/1 that might return an empty list.

The full picture is like that:

https://github.com/nitrogen/nitrogen_core/blob/master/src/nitrogen.erl#L44

https://github.com/nitrogen/nitrogen_core/blob/master/src/lib/wf_handler.erl#L67

The https://www.erlang.org/doc/reference_manual/modules.html says:

attributes

    Returns a list of {AttributeName,ValueList} tuples, where AttributeName is the name of an attribute, and ValueList is a list of values. Notice that a given attribute can occur more than once in the list with different values if the attribute occurs more than once in the module.

    The list of attributes becomes empty if the module is stripped with the beam_lib(3) module (in STDLIB).

Running the above code:

Module = debug_crash_handler, {module, Module} = code:ensure_loaded(Module), L = Module:module_info(attributes).

L will be [] if the debug_crash_handler is stripped.

As we use rebar3 to handle a relase with prod profile, the rebar3 default configuration is to strip all modules. An option
is configure the prod profile like following to avoid stripping modules:

{profiles, [
    %% prod: includes ERTS and system_libs.
    {prod, [
        {relx, [
            {include_erts, true},
            {system_libs, true},

            {debug_info, keep}

        ]}
    ]},

@choptastic
Copy link
Member

Hi @joaohf , this is great!

I have two suggestions to add to this pull request, if you wouldn't mind:

  1. Update the handlers documentation to include the reference and usage of nitrogen:handlers/3: https://github.com/nitrogen/nitrogen_core/blob/master/doc/markdown/handlers.md

  2. Update the error in https://github.com/nitrogen/nitrogen_core/blob/master/src/lib/wf_handler.erl#L92 to mention that if your modules are stripped, you will need to call nitrogen:handlers/3 directly, specifying the type of handler.

Does that work for you?

@joaohf
Copy link
Contributor Author

joaohf commented Jul 13, 2022

Hello @choptastic

Sure. I'll do that.

Thanks.

@joaohf
Copy link
Contributor Author

joaohf commented Jul 16, 2022

@choptastic

I've updated this PR with the first suggestion. But I'm not sure about the second one. Should I update the error on https://github.com/nitrogen/nitrogen_core/blob/master/src/lib/wf_handler.erl#L70 or https://github.com/nitrogen/nitrogen_core/blob/master/src/lib/wf_handler.erl#L92 ?

I think line L70 makes sense because is where the handler is configured at first time. What do you suggest ?

Thanks.

@choptastic
Copy link
Member

You're right, it should be the L70 error message and not my erroneously suggested L92 error. Good catch!

Thanks so far, your documentation changes are great!

There are cases where the beam modules are stripped and is not possible
to get the attribute lists calling Module:module_info(attributes).

So the caller should pass the Name handler as the first parameter.
Avoiding calling the module_info/1 that might return an empty list.
@joaohf
Copy link
Contributor Author

joaohf commented Jul 21, 2022

Hello @choptastic

Ok, here is the patch with the error message. I'm running out of ideas to add a better error message :(

If you have any suggestion, please do.

Thanks.

@choptastic choptastic merged commit 389d720 into nitrogen:master Aug 3, 2022
@choptastic
Copy link
Member

looks great, thanks! I may make a few small tweaks to it, but overall, I really appreciate the work you've done here!

@joaohf joaohf deleted the add-nitrogen-handler-3 branch January 6, 2023 15:16
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