-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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: handle invalid timeout values returned from gen_server callback functions #5683
fix: handle invalid timeout values returned from gen_server callback functions #5683
Conversation
CT Test Results 2 files 83 suites 30m 13s ⏱️ Results for commit 45ca24d. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
I think a macro IIRCC (If I Read the Code Correctly), it seems that your changes in Should |
What if some users calculate timeout dynamically (e.g. read the value from config file)?
In any case (With or without these changes) if we violate the limit the code crashes. The only difference is that with these changes we have a normal crash with complete report: =ERROR REPORT==== 5-Feb-2022::03:25:09.760708 ===
** Generic server m terminating
** Last message in was oops
** When Server state == undefined
** Reason for termination ==
** {bad_return_value,{noreply,undefined,4294967296}}
Yes you're right! I think that the variable name Questions for next changesWe should just go for % inside handle_msg/6 or `handle_common_reply/8 checks:
{noreply, S, Timeout} when ?is_timeout(Timeout) ->
...;
{noreply, S, hibernate} ->
...;
{noreply, S, {continue, _}} ->
...;
% or
-define(
is_timeout_or_hibernate_or_continue(X)
(((X) =:= infinity orelse (is_integer(X) andalso (X) >= 0)) orelse X =:= hibernate orelse (tuple_size(X) == 2 andalso element(1, X) == continue))
).
% ...
% inside handle_msg/6 or `handle_common_reply/8 checks:
{noreply, S, TimeoutOrHibernateOrContinue} when ?is_timeout_or_hibernate_or_continue(TimeoutOrHibernateOrContinue) ->
...;
Should |
If it violates the type ( The simplest solution is to let
Checking for I would prefer to not have one gigantic macro for "is timeout or hibernate or continue", and instead separate them and/or use multiple clauses. I think A warning:
I think that would be nice to have. What should The type spec now only allows |
I see no point. It might be an optimization. I do not know how much of a performance gain it might give... It sure is tempting to remove the clauses from handle_msg, to improve clarity! |
Unfortunately, now when the last 24 maintenance release is out, this PR will have to be rebased to 'master'... After that we have code freeze for 25.rc2 on Friday. Hopefully we can get this in. |
Here (in Iran) we're getting close to Nowruz (It's celebrated on Monday). So l can change things over the next two days and before the holidays. Anything else? |
I can not think of anything else to fix. Regarding optimization I sense that the argument order of So, go ahead and rebase... |
… returned value of callback and enter_loop functions
@@ -617,23 +617,23 @@ enter_loop(Mod, Options, State, {continue, _}=Continue) when is_atom(Mod) andals | |||
Options :: [enter_loop_opt()], | |||
State :: term(), | |||
ServerName :: server_name() | pid(), | |||
Timeout :: timeout() | hibernate | {continue, term()} | |||
Timeout :: timeout() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! Thank you 👍🏼
I forgot to mention that the branch base should also be changed from 'maint' to 'master' in GitHubs GUI, so it looked like a mess for a while... Also, documentation build failed due to |
After an internal discussion we decided to change the code style of guard expressions to use the same style as the rest of the module, that is: traditional comma-and-semicolon separated. |
I think that |
My opinion is that comma-and-semicolon guards are easier to read, when line broken consistently. Probably since I have used Erlang since before So this is a matter of taste, therefore we prefer to keep the style of the module. |
Thank you for your pull request! |
gen_server
module currently does not check returned timeout frominit
and all otherhandle_*
callback functions. IMO in this case it should terminate with a{bad_return_value, _}
too.Example
Before this change
After this change