-
Notifications
You must be signed in to change notification settings - Fork 57
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
Compatibility with new link protocol (and OTP 26) #90
base: master
Are you sure you want to change the base?
Conversation
This PR needs some attention from me i guess, i'll try find a day to review and test this, hope this isn't blocking anyone. |
Not blocking me, at least! I’m fine using the branch. |
I’ll try to check on it this weekend.On 6 Oct 2023, at 03:40, Eric Peterson ***@***.***> wrote:
Not blocking me, at least! I’m fine using the branch.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
|
nice, changes, but how do the exceptions work, won’t that leave it in a unhealthy state? I’ve been swamped with other tasks so I’ve just played a little bit with it and glanced through the code, so sorry if I missed anything obvious. don’t think we would need another process, we can probably handle that with temporary queues. think I’ve done similar things when implementing gen_call. |
I'm also swamped, so it'll take me a bit to respond, but I wanted to say that I've read your comments and appreciate them. I hope to get to this soon. |
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.
Here are some replies. At minimum the raise
s have to be changed; let me know how you all feel about the rest.
I've attempted to address the first round of comments. Again, anyone is welcome to take a look at it. |
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.
Nice, just saw a few more things. Sorry for high latency, not in context with this code so it always takes some time, since this isn’t just a function refactor.
Are you actually using this, is it a big project? just curious on how much milage this has got.
We are using this, but not elaborately. We have a single Pyrlang gen_server wrapper whose job is to make a single Python module available to an Erlang codebase. We've hit that endpoint a million times by now, but each query has been very "vanilla" — single processes on both sides, no weird networking, and just |
@ecpeterson thank you very much for this work! Very interested to see this merge, until then will try to use the code from branch. |
This PR adds handlers for the new link protocol, introduced in OTP 23 and required beginning with OTP 26. Closes #87.
Notes
Signal ordering constraints
I'm concerned by the following line from the ERTS manual:
The most obvious way to guarantee this is to put signal processing on the individual
Process
es and to service them by a single coroutine, guaranteeing ordering generally and the possibility of the immediacy requested here. At present, signal handling is split betweenProcess
es and their hostNode
s, perhaps inviting interleaving. Honestly, I'm not even sure the current state of things (prior to this PR) meets the signal ordering guarantee! On the other hand, I'm not sure this is lethal: I suspect the protocol's correctness is sensitive only to the signal-sandwiching within ~the family of link signals. I'm also not ready to volunteer for a more serious refactoring of signal handling in Pyrlang, should it come to that.There is some related complexity in this PR where I've been defensive against other coroutines modifying a
Process
's state (particularly: its_links
dictionary). We do our best to read-modify-write in betweenawait
s to avoid such races, though this sometimes leads to verbose branching.unlink
It seems that generating an unlink signal to be sent to a remote process was previously unsupported. I've added support for that along the way.
V4_PORT_EXT
OTP 26 also requires an extension to the ETF protocol for deserializing "long" port references. Term and Pyrlang don't have any existing understanding of ports, and port references generally don't get serialized / sent over distribution, so I've guiltlessly advertised the distribution flag that we support that ETF extension without implementing it.
Testing
I've tested this only very lightly.