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 process names, refocus scope, rework API #71

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Add process names, refocus scope, rework API #71

wants to merge 21 commits into from

Conversation

lpil
Copy link
Member

@lpil lpil commented Feb 13, 2025

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.

@lpil lpil marked this pull request as ready for review February 26, 2025 14:03
- 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.

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".

Copy link
Member Author

@lpil lpil Feb 26, 2025

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?

Copy link

@bcpeinhardt bcpeinhardt Feb 26, 2025

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcpeinhardt
Copy link

General note: There's a lot of really good QOL stuff happening in this PR :)

@bcpeinhardt
Copy link

Made a small PR with some minor docs changes so you don't have to bother copying them over: #72

Comment on lines +707 to +712
/// 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.
Copy link
Member

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?

Copy link
Member Author

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]>
Copy link

@rawhat rawhat left a 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 🙏🏻

@lpil lpil mentioned this pull request Feb 28, 2025
}
}
@external(erlang, "erlang", "spawn_link")
pub fn spawn(running: fn() -> anything) -> Pid
Copy link

@ryanmiville ryanmiville Mar 1, 2025

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.

Copy link
Member Author

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.

Copy link

@DanielSherlock DanielSherlock Mar 1, 2025

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great feedback, thank you

@sbergen
Copy link

sbergen commented Mar 1, 2025

Since we're introducing breaking changes, would it make sense to include #67 also? It contains changes from String to Dynamic, and Dynamic to ExitReason.

I can rebase it on top of this branch, if it helps.

@lpil
Copy link
Member Author

lpil commented Mar 1, 2025

Good idea @sbergen! I wonder if there's any further improvements we could make to those types...

@sbergen
Copy link

sbergen commented Mar 1, 2025

Good idea @sbergen! I wonder if there's any further improvements we could make to those types...

I still find the asymmetry of send_abnormal_exit using String somewhat complicated and restrictive. Would rather have a more symmetric send_exit(Pid, ExitReason). Being more symmetric makes things easier to think about (for me, at least), and being able to send a decodable value as an exit reason can be useful. If we also added exit(ExitReason) we'd be more in line with the Erlang exit/1 and exit/2 semantics, as far as I can see.

@lpil
Copy link
Member Author

lpil commented Mar 3, 2025

What are some uses you have for decoding the exit reason?

If we also added exit(ExitReason) we'd be more in line with the Erlang exit/1 and exit/2 semantics, as far as I can see.

This would mean we have it completely unchecked by the type system, and the programmer just have to get the correct magic values!

@sbergen
Copy link

sbergen commented Mar 3, 2025

What are some uses you have for decoding the exit reason?

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 Subject, but if you need to be monitoring a process already, that gets much more complicated than just trying to decode a value IME.

If we also added exit(ExitReason) we'd be more in line with the Erlang exit/1 and exit/2 semantics, as far as I can see.

This would mean we have it completely unchecked by the type system, and the programmer just have to get the correct magic values!

I know, but as I pointed out in #67, the string in Abnormal(String) could be just about anything right now also (which is why Abnormal(Dynamic) makes more sense). I don't think there's anything we can do about processes being able to panic with an unexpected value: if not from Gleam code with e.g. let assert (I proposed making GleamError a type you could decode to also, but that suggestion wasn't popular), then from Erlang code causing the process to error/exit.

I guess some kind of API where you could have typed processes, Pid(a), and ExitReason(a) with some extra variant could perhaps be made to work, but that would be a much bigger change, and I somehow feel it would be more trouble than it would be worth.

@lpil
Copy link
Member Author

lpil commented Mar 4, 2025

It sounds like a mistake to me to be using non-local error handling in such a fine-grained way.

I know, but as I pointed out in #67, the string in Abnormal(String) could be just about anything right now also (which is why Abnormal(Dynamic) makes more sense). I don't think there's anything we can do about processes being able to panic with an unexpected value: if not from Gleam code with e.g. let assert (I proposed making GleamError a type you could decode to also, but that suggestion wasn't popular), then from Erlang code causing the process to error/exit.

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 panic.

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.

I guess some kind of API where you could have typed processes, Pid(a), and ExitReason(a) with some extra variant could perhaps be made to work, but that would be a much bigger change, and I somehow feel it would be more trouble than it would be worth.

Pids are not typable with their messages as it makes call impossible to implement, and exits are not typable as they can originate from outside of Gleam, just like exceptions.

Now is the time to make large changes! There will be no other time to do so.

@lpil
Copy link
Member Author

lpil commented Mar 5, 2025

OK, I am convinced @sbergen ! I have merged your changes into this branch. :)

///
pub fn selecting_process_down(
pub fn selecting_specific_monitor(

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.

Copy link
Member Author

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.

@sbergen
Copy link

sbergen commented Mar 5, 2025

It sounds like a mistake to me to be using non-local error handling in such a fine-grained way.

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...

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 panic.

...but indeed, this sort of information could be included in strings also. I just like types a lot more than strings :)

...and exits are not typable as they can originate from outside of Gleam, just like exceptions.

I think you could have one typed variant of ExitReason, but especially if we don't want to encourage using exit values, I don't think it's really an avenue worth pursuing.

OK, I am convinced @sbergen ! I have merged your changes into this branch. :)

send_abnormal_exit still takes a string, but I think as long as Abnormal holds Dynamic, it will make using dynamic exit values with FFI for those who really want to do it easy enough 👍

Comment on lines +116 to +117
/// Create a subject for a name, which can be used to send and receive messages.
///
Copy link

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.

Copy link
Member Author

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)
Copy link
Contributor

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).

Copy link
Member Author

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?

Copy link
Contributor

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
///
Copy link
Contributor

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: ...)
}

Copy link
Contributor

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
Copy link
Contributor

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]))).
Copy link
Contributor

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)
Copy link
Contributor

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).

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.

8 participants