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

Compatibility with new link protocol (and OTP 26) #90

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ecpeterson
Copy link

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:

Upon reception of an UNLINK_ID signal, the corresponding UNLINK_ID_ACK signal must be sent before any other signals are sent to the sender of the UNLINK_ID signal.

The most obvious way to guarantee this is to put signal processing on the individual Processes 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 between Processes and their host Nodes, 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 between awaits 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.

@kvakvs
Copy link
Contributor

kvakvs commented Oct 6, 2023

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.

@ecpeterson
Copy link
Author

Not blocking me, at least! I’m fine using the branch.

@s2hc-johan
Copy link
Collaborator

s2hc-johan commented Oct 6, 2023 via email

@s2hc-johan
Copy link
Collaborator

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.

pyrlang/process.py Show resolved Hide resolved
pyrlang/node.py Show resolved Hide resolved
pyrlang/node.py Outdated Show resolved Hide resolved
@ecpeterson
Copy link
Author

ecpeterson commented Oct 26, 2023

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.

Copy link
Author

@ecpeterson ecpeterson left a 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 raises have to be changed; let me know how you all feel about the rest.

pyrlang/process.py Show resolved Hide resolved
pyrlang/node.py Outdated Show resolved Hide resolved
pyrlang/node.py Show resolved Hide resolved
@ecpeterson
Copy link
Author

I've attempted to address the first round of comments. Again, anyone is welcome to take a look at it.

Copy link
Collaborator

@s2hc-johan s2hc-johan left a 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.

pyrlang/node.py Outdated Show resolved Hide resolved
pyrlang/node.py Outdated Show resolved Hide resolved
@ecpeterson
Copy link
Author

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 gen_server:call/2 over and over.

pyrlang/node.py Outdated Show resolved Hide resolved
pyrlang/node.py Outdated Show resolved Hide resolved
pyrlang/node.py Outdated Show resolved Hide resolved
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.

Incompatible with OTP 26.0
4 participants