Server protocol improvements #36
Conversation
There was a problem hiding this comment.
Note that you can make "a PR against another PR" basically (if you want to just see the relevant changes here).
This lumps together several changes and might be better served with individual tickets? e.g. come to a conclusion on the "closing" stuff, and then update the spec. Similar for "released", "list" etc.
I haven't thought hard about the backwards / forwards compatibility concerns of all these changes, but don't see any discussion about it: that is, what happens when a new client is on one side and an old one on the other? What about a new client talking to an "old" server? Do they need to know and behave differently? That is, there are several cases to consider: old vs new server and matched new, matched old, or mismatched peers.
It's in one comment, but I also think the spec could benefit from making more clear what are current behaviors vs. "planned future changes". Ideally by linking to tickets (especially because without tickets on the server, these changes may never happen ;) ).
|
p.s. overall it's great to move this stuff forward, but I don't want to end up with a "spec" that has zero implementations -- this is partly why I guess I'm suggesting separate tickets, or at least words that indicate the status (and link to e.g. "implement this in the server" tickets). |
|
Thanks for the feedback. I'll go ahead and start implementing stuff server side soon. Also I'll remove the "session shutdown" related changes as they clearly require some more discussion. |
piegamesde
force-pushed
the
mailbox-simplification
branch
from
d9ce904 to
92a2224
Compare
piegamesde
force-pushed
the
mailbox-simplification
branch
2 times, most recently
from
2ca71dc to
433fe76
Compare
|
I've removed quite a few of the proposed features, as they turned out to be more complicated than anticipated. There is now a pull request at the server repository, which should implement all the changes proposed in here: magic-wormhole/magic-wormhole-mailbox-server#38 |
The server has already opened the mailbox at this point so there is a mailbox ID, but the client would still need to issue an `open` in order to access it. Now the "allocated" response also contains the mailbox ID, so clients may save a round trip if they want to.
piegamesde
force-pushed
the
mailbox-simplification
branch
from
433fe76 to
484a76b
Compare
|
Upon re-reading, I do not understand why we'd want to change the protocol to make CLAIM not allocate a mailbox-id? Why would a client want to CLAIM a nameplate, but not allocate a mailbox-id? If they did do a CLAIM with "allocate=False", how do they allocate a mailbox-id? If there are scenarios that these changes are intended to fix, can you either file or link to relevant tickets please? |
this is for the second party, under the assumption that a mailbox for that nameplate has already been allocated. |
I see, so you mean for a second party that has typed the code in wrong (with the incorrect nameplate)? (Regardless of the outcome of this "protocol" PR, are there tickets filed anywhere else that we can refer to?) |
(This builds on top of the changes in #32 and thus depends on that being merged first.)
Numerous small changes to the server protocol that either better document existing behavior or change the semantics.
I tried to keep the diff as minimal while stating the intent of the changes, but the entire file is pending a rewrite and adding diagrams. I plan on doing that later, as I want to get agreement on the changes themselves first.
None of these changes should be client-breaking, except for the fact of sending
closedbeforeclosefor the mailbox half-closing mechanism. That, I am unsure about. Server side, some things will need to be double checked, but I hope it should be generally fine too.