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.

Sign up for GitHub

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

Jump to bottom

gen_server types (OTP-17915) #5751

Merged
merged 8 commits into from Mar 14, 2022
Merged

gen_server types (OTP-17915) #5751

merged 8 commits into from Mar 14, 2022

Conversation

RaimoNiskanen
Copy link
Contributor

This pull request exports gen_server types as suggested in PR #2375 and PR #2690, fixes a few type violations now found in Kernel, updates the documentation to utilize the type specs, and hopefully gets all type details right, at least after a few rounds of review...

Is this the right set of types to export from gen_server?
Are the right types opaque in the right way?
Is the documentation update an improvement?
Are there any obvious things to fix in the documentation while at it?

@RaimoNiskanen RaimoNiskanen added testing currently being tested, tag is used by OTP internal CI types The issue is related to types enhancement in progress priority:medium labels Feb 25, 2022
@RaimoNiskanen RaimoNiskanen self-assigned this Feb 25, 2022
@RaimoNiskanen RaimoNiskanen added team:PS Assigned to OTP team PS team:VM Assigned to OTP team VM labels Feb 25, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 25, 2022 •
edited

CT Test Results

       4 files     159 suites   1h 27m 47s ⏱️
3 061 tests 2 869 ✔️ 191 💤 1 ❌
3 559 runs  3 330 ✔️ 228 💤 1 ❌

For more details on these failures, see this check.

Results for commit 95f0ec3.

♻️ 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

@bjorng
Copy link
Contributor

bjorng commented Feb 27, 2022

This pull request causes the behaviour_SUITE:gen_server_incorrect_args/1 and small_SUITE:gencall/1 test cases for the dialyzer application to fail.

@RaimoNiskanen RaimoNiskanen added testing currently being tested, tag is used by OTP internal CI and removed testing currently being tested, tag is used by OTP internal CI labels Feb 28, 2022
@RaimoNiskanen
Copy link
Contributor Author

To facilitate documentation review, here are some links of what I feel would be worth looking at:

  • Data Types
  • start_link/3,4 that refers to type start_ret() for its return value instead of describing them, as does all start*/* functions.
  • A perhaps new way of describing the return values for Module:handle_call/3
  • Module:init/1 that refers to Module:handle_call/3 for describing the Timeout values.

And, of course, If anyone would proofread the whole new gen_server documentation that would be appreciated...

@RaimoNiskanen
Make gen_server doc use type specs
0e2da2d 
Since there now are type specs I have moved the explanation of
of the type values to the type.  Previously the it had to be
under a selected function and then other functions referred
to that function.  Now all functions refer to the type.

For example; under the type start_ret() the possible return
values for all start functions (start/3,4, start_link/3,4
and start_monitor/3,4) are described and all start functions
refer to start_ret().  Previously the return values were
described under start_link/3,4 and the other start functions
referred to it.

Now you often need to follow a link to find the explanation
of a value, but it is described together with the auto
generated type spec so it is less likely that the description
starts diverting from the type spec, and the description
can refer to annotation variables in the spec.

In short, information duplication has been reduced by
moving the information to a type, but that requires
the reader to follow a link to find the information
from a function description.
@RaimoNiskanen
Update dialyzer test cases
76bed16
@RaimoNiskanen RaimoNiskanen force-pushed the raimo/stdlib/gen_server-types/GH-2375_GH-2690/OTP-17915 branch from e97026b to 76bed16 Compare March 2, 2022 13:56
@RaimoNiskanen RaimoNiskanen added testing currently being tested, tag is used by OTP internal CI and removed testing currently being tested, tag is used by OTP internal CI labels Mar 2, 2022
@IngelaAndin
Copy link
Contributor

In the documentation of the function gen_server:call it says:

If no reply is received within the specified time, the function call fails. If the caller catches the failure and continues running, and the server is just late with the reply, it can arrive at any time later into the message queue of the caller.

I never liked this wording. I think "the function call fails" is too vague and it is not obvious what is meant by that. I think it would be clearer to say the function calls fails by exiting the process that is calling the function, and if the process catches this exit and continues to run it must be prepared to receive a late answer, as the server is not affected by the client timeout. This should be done by disregarding any such late messages that are two-element tuples with a reference as the first element.

Also, I think it would be an improvement if we could make an exit type for the most common exit reasons instead of just saying

"The call can fail for many reasons, including time-out and the called gen_server process dying before or during the call."

@IngelaAndin
Copy link
Contributor

Also, enter_loop could specify its failing by exiting better.

@RaimoNiskanen
Copy link
Contributor Author

Also, enter_loop could specify its failing by exiting better.

That is discussed in PR #5683. I think we should handle it there.

@RaimoNiskanen
Copy link
Contributor Author

Also, I think it would be an improvement if we could make an exit type for the most common exit reasons instead of just saying

Does it really merit a type of its own?
But I agree we could take the opportunity to document it.

@IngelaAndin
Copy link
Contributor

Also, enter_loop could specify its failing by exiting better.

That is discussed in PR #5683. I think we should handle it there.

Sure, you will be the driver either way ;)

@IngelaAndin
Copy link
Contributor

Also, I think it would be an improvement if we could make an exit type for the most common exit reasons instead of just saying

Does it really merit a type of its own? But I agree we could take the opportunity to document it.

Humm ... maybe, maybe not (it is not a return-type after all). Make a suggestion and we can take it from there.

@RaimoNiskanen RaimoNiskanen removed the testing currently being tested, tag is used by OTP internal CI label Mar 8, 2022
@RaimoNiskanen RaimoNiskanen force-pushed the raimo/stdlib/gen_server-types/GH-2375_GH-2690/OTP-17915 branch from ee30150 to 5e3e003 Compare March 8, 2022 15:54
@RaimoNiskanen RaimoNiskanen added the testing currently being tested, tag is used by OTP internal CI label Mar 8, 2022
@RaimoNiskanen
Clean up handling of gen exits in gen_statem
414bf11 
The `gen` module raises `exit` exceptions when it
detects an error.  Clean up handling of these by
not handling other classes of errors, to not
accidentally bury an interesting cannot-happen-error.
@RaimoNiskanen
Reword gen_server:call exit description
b92c00f
@RaimoNiskanen
Reword gen_statem:call exit description
95f0ec3
@RaimoNiskanen RaimoNiskanen force-pushed the raimo/stdlib/gen_server-types/GH-2375_GH-2690/OTP-17915 branch from 5e3e003 to 95f0ec3 Compare March 9, 2022 11:34
@RaimoNiskanen RaimoNiskanen added testing currently being tested, tag is used by OTP internal CI and removed testing currently being tested, tag is used by OTP internal CI labels Mar 9, 2022
@RaimoNiskanen RaimoNiskanen added testing currently being tested, tag is used by OTP internal CI and removed testing currently being tested, tag is used by OTP internal CI labels Mar 10, 2022
@RaimoNiskanen RaimoNiskanen merged commit f0883ac into erlang:master Mar 14, 2022
This was referenced Mar 14, 2022
Closed
Closed
@RaimoNiskanen RaimoNiskanen linked an issue Mar 16, 2022 that may be closed by this pull request
specs for gen_server module #5364
Closed
@RaimoNiskanen RaimoNiskanen mentioned this pull request Mar 16, 2022
Closed
@RaimoNiskanen RaimoNiskanen deleted the raimo/stdlib/gen_server-types/GH-2375_GH-2690/OTP-17915 branch March 18, 2022 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgud dgud dgud approved these changes

@IngelaAndin IngelaAndin IngelaAndin approved these changes

@garazdawi garazdawi Awaiting requested review from garazdawi

@rickard-green rickard-green Awaiting requested review from rickard-green

@HansN HansN Awaiting requested review from HansN

Assignees

@RaimoNiskanen RaimoNiskanen

Labels
enhancement in progress priority:medium team:PS Assigned to OTP team PS team:VM Assigned to OTP team VM types The issue is related to types
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

specs for gen_server module
4 participants
@RaimoNiskanen @bjorng @IngelaAndin @dgud