Improvements #38
Improvements #38
Conversation
This avoids clients to have to call `claim` after `allocate` just in order to know their mailbox.
piegamesde
force-pushed
the
master
branch
2 times, most recently
from
e63c317 to
c345703
Compare
piegamesde
force-pushed
the
master
branch
4 times, most recently
from
aa69e59 to
97746fb
Compare
Otherwise, one cannot retry after a call failed, because all succeeding calls return "only one claim per connection"
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #38 +/- ##
==========================================
+ Coverage 97.10% 97.13% +0.03%
==========================================
Files 8 8
Lines 829 839 +10
Branches 140 143 +3
==========================================
+ Hits 805 815 +10
Misses 16 16
Partials 8 8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| self["websocket-protocol-options"] = [] | ||
| self["allow-list"] = True | ||
| self["allow-list"] = False |
There was a problem hiding this comment.
I don't really want to flip around the CLI args like this -- it breaks everyone's command-lines with very little gain.
Instead we could suggest to operators that they should prefer to use --disallow-list in the immediate term, and that clients should not build in functionality that depends on list working all the time.
There was a problem hiding this comment.
Um, not sure how this may break command lines? This was not my intention
There was a problem hiding this comment.
This flips things around to turn list off by default. So if operators already wanted list to work, they'll have to change their command-lines (since the default changes).
There was a problem hiding this comment.
Well yes, that's how "enabling/disabling a feature by default" works? I wouldn't consider that breaking, but I can change it back if you don't like it
There was a problem hiding this comment.
How about this: I keep the current default value, but also add the allow-list flag and print a warning that one should use it to keep that behavior in the future.
There was a problem hiding this comment.
Yeah, lets leave the flipping around of CLI flags alone in this PR. There's already a way to run with list on, and a way to run with list off.
(The CLI is the only public interface here, so I don't see a lot of gain in changing it merely to communicate "you should probably turn off list by default").
|
Deployment considerations: I think we have six possible configurations, if we ignore which "side" of the two peers is "new" versus "old" (otherwise there are more combinations). We can have an old or new server and old or new peers (where the relevant distinction is "are they the same or different"). So we have "both-old" peers, "both-new" peers and "mixed" peers. The six cases are:
So, for the new argument ("allocate=False") to Same for the proposed new flag to |
|
(Also: I really do want to ignore this until after Dilation improvements are completed as I don't think it's relevant to that, directly). |
Companion PR of magic-wormhole/magic-wormhole-protocols#36
I'm opening this mostly for CI atm, as I don't really have a development environment for this project