Make multilocale libraries determine their port information on the fly #13044
Conversation
Doesn't send the client's setup socket information, but tries to perform the handshake for the main, arg, and res ports. Unfortunately, this currently runs into a segfault, but I'm going to figure that out next.
… connect Was able to get the server to call the function. Segfault occurred during the call to chpl_library_finalize.
I did this through a new flag, --mli-socketLoc, which gets appended to the end of the original argv and then pulled before entering chpl_library_init on the server side. I am not married to the name, and there might be a better strategy for it.
This should hopefully avoid any possibility of naming conflicts
|
It's strange to me that we have to use a flag to sneak our information through the launcher. In normal execution of Chapel multi-locale libraries, the end user has to be able to pass through an unadorned argv/argc to the entrypoint, no? What is preventing us from just leveraging that? Just curious to know a bit more about how the launcher whines at us. |
|
Basically, what was going on was that |
|
Ok, sounds reasonable. Thanks for clarifying. My only worry is making sure a user can't (whether accidentally or maliciously) get ahold of the flag somehow? |
|
That's a good point. I should add some additional checking |
There was a problem hiding this comment.
Looks good to me.
Mainly just seeing refactoring opportunities that are fallout from decisions I made clashing against the new design.
I think we should try to stick to a coding style internally, but I'll leave it to you to decide what that is (or if we even need to bother).
Right now I'm just worried about if a user abuses that internal flag! You have something in the works for that, I'm sure 👍.
| // Drop down into the server listen loop, passing in the last argument, which | ||
| // will be the socket connection information for communicating where the | ||
| // server will be listening. | ||
| chpl_mli_smain(argv[argc-1]); |
There was a problem hiding this comment.
Hmmm, I'm thinking that we might want to refactor the listen loop into "setup" -> "listen" -> "teardown" functions. Not necessarily with this PR, though.
There was a problem hiding this comment.
It's good that we continue to keep the actual body of main as small as possible, though.
- If we see it twice, that's because the user added it themselves, so we tell them it is for internal use only - If we only see the flag and it is the last argument (not followed by the connection information), tell them it needs to be followed by connection information - If we see the flag and what follows it does not start with `tcp://`, then it is likely being used improperly (either by not having connection information follow it, or by having a bad string follow it), so tell the user not to. In any case, only the last use of the flag would matter in a multilocale library run (other uses get dropped on the floor) and we ensure that we provide the last use of the flag when we look for it, so it shouldn't pose a problem.
For the purposes of the client or server's use of ZMQ in a multilocale library, we intend to only bind ephemerally, and then send that information to the pair that will be connecting. So the bind call does not need anything other than the socket that will be bound.
…consistent) Use snake_case instead of an inconsistent mixture of snake_case and camelCase
- Capitalize first letter of every debug message - Use a colon in the first chpl_mli_push_connection debug message
|
I think I've addressed your feedback (except for the one I responded to directly just now). I need to:
|
|
But I think it would be good for you to take a look at what's here now, and let me know if you think any of those three tasks shouldn't hold up the PR (currently leaning towards number 3 not holding it up) |
|
(current branch passed full paratest with futures for CHPL_COMM=none, so I didn't break that at least) |
|
It looks good, I like the error messages in As for what you told me earlier about the VPN issue, I'd be OK with making a separate issue to explore what's going on there. |
This change modifies the generation of multilocale libraries to use
ephemeral ports instead of hard-coded ones when communicating
between the client and spawned server. It does this via a secret
flag that is not visible to the user,
--chpl-mli-socket-loc, whichcommunicates the connection information where the client will
listen. The server will then create sockets for communicating the
function calls, arguments, and return values of the exported
functions, and will use the client's connection socket to
communicate the connection information for those sockets.
Passed testing over std/ paratest with futures and a full gasnet paratest.