-
-
Notifications
You must be signed in to change notification settings - Fork 41
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 process names, refocus scope, rework API #71
base: main
Are you sure you want to change the base?
Conversation
- The `create_from_string` function has been renamed to `create`. | ||
- In the `gleam/erlang/node` module: | ||
- The `send` function has been renamed to `untyped_send`. | ||
- The `gleam/erlang` module has been removed. |
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.
Trying to mentally map where to reach for functionality that lived here previously, only one I see us getting consistent questions about is get_line
. May be worth including a suggested replacement in the changelog, even if it's "include this small ffi snippet".
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.
I don't have any recommendations here! Is there a package we can point to?
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.
I couldn't find one. My suspicion is there's a reason it's hard to do, but I suppose the only thing to do is make one and find out why it's hard. Honestly I can take a stab at it this evening :)
Edit: That was easy I made one on my lunch break.
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.
General note: There's a lot of really good QOL stuff happening in this PR :) |
Made a small PR with some minor docs changes so you don't have to bother copying them over: #72 |
/// This function will panic under the following circumstances: | ||
/// - The callee process exited prior to sending a reply. | ||
/// - The callee process did not send a reply within the permitted amount of | ||
/// time. | ||
/// - The subject is a named subject but no process is registered with that | ||
/// name. |
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.
Why is this function panicking instead of returning an error?
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.
In BEAM programs we have two types of error handling, local with values, and non-local with supervision. Non-local is for unexpected errors that are outside the program domain, things that shouldn't ever happen and must be enforced. A call failing is considered to be closer to a stack overflow or a heap allocation failure than validation failing, so it uses non-local error handling.
There was previously versions that used errors alongside these but they had to be used with extreme caution as once the call had failed the caller process could be left one of several undesirable states, and you would need to carefully handle these to ensure you don't leak any messages, which is both a memory leak and something that slows performance each time it happens. I couldn't find a single instance of it being done correctly on GitHub, and people preferred it because we have trained them that using results is the best option, so that convinced us that it was not the correct API to offer. In future perhaps we could discover some other API that removes these errors.
Co-authored-by: Giacomo Cavalieri <[email protected]>
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.
i like these changes. RIP rescue
🙏🏻
} | ||
} | ||
@external(erlang, "erlang", "spawn_link") | ||
pub fn spawn(running: fn() -> anything) -> Pid |
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.
Do you plan to follow this convention with gleam_otp v1? start
and start_unlink
rather than start
and start_link
? I think either way is fine as long as both packages are consistent.
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.
I intend to use start
and to link by default. What the unlinked API would be is not yet decided, but if it's two functions they'll be named like this.
src/gleam/erlang/node.gleam
Outdated
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.
I mentioned in discord that I really like the short summaries in the other files about such-and-such Erlang feature. It massively helps me as someone who is unfamiliar with all these Erlangy words to get a sense of what each module is about.
node.gleam
doesn't have one of these. In hindsight I should have been able to piece together what a node is from the other function names and their types and docs in this module... but I didn't, and if I'd have known up top that:
- I should think of multiple nodes as multiple BEAM instances that can communicate (each running probably a whole bunch of processes), and
- Other nodes might be using a different (version of the) codebase, so I can't guarantee that the types match up,
Then I would have already had the context I needed to quickly notice the differences between node.untyped_send
and process.send
and understand why those differences exist. I hope this perspective is useful. And obviously I don't know if my "facts" above are exactly correct but I'm sure you'll write them better than I can anyway.
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.
Great feedback, thank you
Since we're introducing breaking changes, would it make sense to include #67 also? It contains changes from I can rebase it on top of this branch, if it helps. |
Good idea @sbergen! I wonder if there's any further improvements we could make to those types... |
I still find the asymmetry of |
What are some uses you have for decoding the exit reason?
This would mean we have it completely unchecked by the type system, and the programmer just have to get the correct magic values! |
The uses I've had would be to report a more structured error from a process that should terminate. E.g. when using a process to communicate with a server, where the protocol specifies that the connection should be immediately closed on any protocol violations, it would be nice to be able to report if the reason to terminate was due to a) a protocol violation from the server, or b) one of our own assertions about situations that shouldn't happen (e.g. Erlang timers often lead me to add assertions like this). You could argue that these could/should be reported through a
I know, but as I pointed out in #67, the string in I guess some kind of API where you could have typed processes, |
It sounds like a mistake to me to be using non-local error handling in such a fine-grained way.
The message is a debug convenience rather than a tool for exception based flow control, the same as the text message one can give to a Something being unstructured is not an argument in favour of removing useful structure from a related thing. The Erlang design is informative but replicating it is not a goal as we are focusing on types and making invalid use impossible, while the Erlang API attempts neither.
Pids are not typable with their messages as it makes Now is the time to make large changes! There will be no other time to do so. |
OK, I am convinced @sbergen ! I have merged your changes into this branch. :) |
/// | ||
pub fn selecting_process_down( | ||
pub fn selecting_specific_monitor( |
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.
Is 'specific' needed? I think selecting_monitor
is already clear since it's singular and you pass the monitor.
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.
Yes, it's to discourage its use.
As a package author, I was thinking of it more as a HTTP 500 (oops, my bad) vs 502 (I can't do my job because of a third party) that could be reported to the user of the package...
...but indeed, this sort of information could be included in strings also. I just like types a lot more than strings :)
I think you could have one typed variant of
|
/// Create a subject for a name, which can be used to send and receive messages. | ||
/// |
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.
While the subject_owner
and send
docs indirectly indicate how this works, I feel it could be beneficial to be more specific with the docs here also.
If I got this right, something along the lines of
Create a subject for a name, which can be used by the process registered with this name to receive messages, and other processes to send messages to the named process.
Also, I can't help but wonder about multiple instances. I assume it's along the lines of:
All subjects created with the same name behave identically to each other.
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.
Great feedback, thank you
/// Create a subject for a name, which can be used to send and receive messages. | ||
/// | ||
pub fn named_subject(name: Name(message)) -> Subject(message) { | ||
NamedSubject(name) |
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.
With reservation for not completely understanding this, it looks as if you only can have one Subject for a named process. How would you create different subjects for a named process?
The new_subject creates a new reference each time you call it, which means you may have several Subjects (Message sets).
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.
A process can have as many unnamed subjects as they want (useful for ad-hoc messages such as the reply in call
), but each process can only have one name, and the API of that name is a single message type. To me this seemed in keeping with how Erlang names are used.
Perhaps I am making a mistake here and this is too restrictive. Do you have thoughts on how one may want to use multiple distinct subjects for one name?
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.
OK, thanks.
That makes sense, and references in tags will not survive a restart, so you would have to re-register those as well as the process id. I guess that if you have a service that present interfaces for different types of clients (using different protocols) this could be of interest, but maybe you just solve it by presenting Subject handles to the different interfaces from the "single API".
@@ -64,19 +70,72 @@ fn spawn_link(a: fn() -> anything) -> Pid | |||
/// |
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.
Is there a reason to not having an Owner type and keep Subject type as is?
pub type Owner {
Pid(owner: Pid)
Named(name: ...)
}
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.
OK, a reference is "node-local". Maybe this is the reason. Which would apply to my previous comment too.
case named(name) { | ||
Ok(pid) -> { | ||
raw_send(pid, #(name, message)) | ||
Nil |
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.
Silently dropping for unregistered pid is fine for me, but it differs from the Erlang implementation.
21> register(hej, self()).
true
22> erlang:send(hej, hoj).
hoj
23> erlang:send(hj, hoj).
** exception error: bad argument
in function erlang:send/2
called as erlang:send(hj,hoj)
*** argument 1: invalid destination
``
{error, {unknown_application, ProblemApp}} | ||
end. | ||
new_name() -> | ||
list_to_atom("name" ++ integer_to_list(erlang:unique_integer([positive]))). |
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.
Let the programmer decide the name, so the process will be easy to find with inspection/debugging tools, but keep appending the unique number.
/// entire virtual machine to crash. | ||
/// | ||
@external(erlang, "gleam_erlang_ffi", "new_name") | ||
pub fn new_name() -> Name(message) |
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.
Change new_name() to new_name(name: String) so that it will be possible to give descriptive labels.
The function shall still append a unique number to the name (for uniqueness).
Reworking the API of this package for a v1 release! The big new feature is named processes, which will make writing OTP programs much easier in Gleam.