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 checkout stacktrace when noproc #94

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fishcakez
Copy link
Contributor

gen_server:cast/2 can set the last stacktrace as it will catch the badarg if a locally named process is not registered.

gen_server:cast/2 can set the last stacktrace as it will catch the
badarg if a locally named process is not registered.
@getong
Copy link
Contributor

getong commented Aug 15, 2018

The erlang:get_stacktrace/0 function keeps the latest error stacktrace info, no matter what code place it is in. You can try it in the shell, and you can still get the same error info after several code operations, until you make another error in the shell.
The pr can be closed now.

@fishcakez
Copy link
Contributor Author

@getong I think you misunderstand the issue. gen_server:cast/2 will set a new stacktrace in some situations but won't raise. The erlang:get_stacktrace/0 call needs to be done before gen_server:cast/2 so that the correct stacktrace is used to reraise. This bug was not fixed in #104 except for OTP 21+. It should still be fixed for early versions.

@getong
Copy link
Contributor

getong commented Aug 15, 2018

Ther erlang:get_stacktrace/0 function only keeps the most latest error info, no matter what place it is put into. You can try this code below:

try 1/0
    catch
        _:_ ->
            A = erlang:get_stacktrace(),
           %% you can do something else here   
            io:format("test1\n", []),
            B = erlang:get_stacktrace(),
            io:format("test2\n", []),
            C = erlang:get_stacktrace(),
            case A == B andalso B == C of
                true ->
                    io:format("A equals B and equals C\n", []);
                false ->
                    io:format("A equals B and equals C\n", [])
            end
    end.

The result is

test1
test2
A equals B and equals C

@fishcakez
Copy link
Contributor Author

Thats correct but we want the stacktrace from gen_server:call/3, not gen_server:cast/2. Therefore must call erlang:get_stacktrace/0 before gen_server:cast/2, to get the stacktrace from gen_server:call/3. If we call it after gen_server:cast/2 (as now) the stacktrace may have been replaced by an error inside gen_server:cast/2, which produces a confusing error, as the class and reason do not match the stacktrace.

@fishcakez
Copy link
Contributor Author

To put this in a similar way to your code example, the issue here is that A == B may not always be true because an exception can be caught (and silently handled) inside gen_server:cast/2.

try
    gen_server:call(Pool, {checkout, CRef, Block}, Timeout)
catch
    Class:Reason ->
        A = erlang:get_stacktrace(),
        gen_server:cast(Pool, {cancel_waiting, CRef}),
        B = erlang:get_stacktrace(),
        A == B
end

@getong
Copy link
Contributor

getong commented Aug 15, 2018

If gen_server:cast(Pool, {cancel_waiting, CRef}), causes a error, then it crashes, and it won't run into the erlang:raise(Class, Reason, erlang:get_stacktrace()).

@fishcakez
Copy link
Contributor Author

The gen_server:cast/2 implementation is approximately:

try
    do_send(prod, msg)
catch
    _:_ ->
        ok
end

So any exception raised is caughted, the stacktrace replaced for that process and flow continues. Therefore erlang:raise/3 will be called with the incorrect stacktrace when an exception is raised inside gen_server:cast/2.

@getong
Copy link
Contributor

getong commented Aug 15, 2018

Ok, I think you are right, and this pr can be merged.

@getong
Copy link
Contributor

getong commented Aug 17, 2018

@fishcakez
Would you rebase to current master branch?

GlenWalker added a commit to GlenWalker/poolboy that referenced this pull request Jan 4, 2019
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