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.

Sign up for GitHub

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

Improvements #38

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Improvements #38

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c88c9c6
Tell the mailbox already in the `allocated` message
piegamesde Mar 26, 2023
5b69792
Opt-out of nameplate allocation in `claim` with optional parameter
piegamesde Mar 31, 2023
a01418b
Disallow listing nameplates by default
piegamesde Mar 31, 2023
63b1144
Fix error recovery after failed `claim`
piegamesde Mar 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/wormhole_mailbox_server/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ class CrowdedError(Exception):
pass
class ReclaimedError(Exception):
pass
class UnknownNameplateError(Exception):
pass

Usage = namedtuple("Usage", ["started", "waiting_time", "total_time", "result"])
TransitUsage = namedtuple("TransitUsage",
Expand Down Expand Up @@ -234,10 +236,9 @@ def _find_available_nameplate_id(self):
def allocate_nameplate(self, side, when):
nameplate_id = self._find_available_nameplate_id()
mailbox_id = self.claim_nameplate(nameplate_id, side, when)
del mailbox_id # ignored, they'll learn it from claim()
return nameplate_id
return nameplate_id, mailbox_id

def claim_nameplate(self, name, side, when):
def claim_nameplate(self, name, side, when, allow_allocate=True):
# when we're done:
# * there will be one row for the nameplate
# * there will be one 'side' attached to it, with claimed=True
Expand All @@ -250,6 +251,8 @@ def claim_nameplate(self, name, side, when):
" WHERE `app_id`=? AND `name`=?",
(self._app_id, name)).fetchone()
if not row:
if not allow_allocate:
raise UnknownNameplateError("Claimed nameplate does not exist, and `allow_allocate` is set to False")
if self._log_requests:
log.msg("creating nameplate#%s for app_id %s" %
(name, self._app_id))
Expand Down Expand Up @@ -676,7 +679,7 @@ def stopService(self):
app._shutdown()
return service.MultiService.stopService(self)

def make_server(db, allow_list=True,
def make_server(db, allow_list=False,
advertise_version=None,
signal_error=None,
blur_usage=None,
Expand Down
12 changes: 9 additions & 3 deletions src/wormhole_mailbox_server/server_tap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,22 @@ class Options(usage.Options):
("motd", None, None, "Send a Message of the Day in the welcome"),
]
optFlags = [
("disallow-list", None, "refuse to send list of allocated nameplates"),
("disallow-list", None, "default. refuse to send list of allocated nameplates"),
("allow-list", None, "allow to send list of allocated nameplates"),
]

def __init__(self):
super(Options, self).__init__()
# Default values
self["websocket-protocol-options"] = []
self["allow-list"] = True
self["allow-list"] = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um, not sure how this may break command lines? This was not my intention

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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").


def opt_disallow_list(self):
self["allow-list"] = False
# no-op because default
pass

def opt_allow_list(self):
self["allow-list"] = True

def opt_log_fd(self, arg):
self["log-fd"] = int(arg)
Expand Down
18 changes: 12 additions & 6 deletions src/wormhole_mailbox_server/server_websocket.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from twisted.internet import reactor
from twisted.python import log
from autobahn.twisted import websocket
from .server import CrowdedError, ReclaimedError, SidedMessage
from .server import CrowdedError, ReclaimedError, UnknownNameplateError, SidedMessage
from .util import dict_to_bytes, bytes_to_dict

# The WebSocket allows the client to send "commands" to the server, and the
Expand Down Expand Up @@ -69,7 +69,7 @@
# -> {type: "list"} -> nameplates
# <- {type: "nameplates", nameplates: [{id: str,..},..]}
# -> {type: "allocate"} -> nameplate, mailbox
# <- {type: "allocated", nameplate: str}
# <- {type: "allocated", nameplate: str, mailbox: str}
# -> {type: "claim", nameplate: str} -> mailbox
# <- {type: "claimed", mailbox: str}
# -> {type: "release"}
Expand Down Expand Up @@ -185,27 +185,33 @@ def handle_list(self):
def handle_allocate(self, server_rx):
if self._did_allocate:
raise Error("you already allocated one, don't be greedy")
nameplate_id = self._app.allocate_nameplate(self._side, server_rx)
nameplate_id, mailbox_id = self._app.allocate_nameplate(self._side, server_rx)
assert isinstance(nameplate_id, type(""))
self._did_allocate = True
self.send("allocated", nameplate=nameplate_id)
self.send("allocated", nameplate=nameplate_id, mailbox=mailbox_id)

def handle_claim(self, msg, server_rx):
if "nameplate" not in msg:
raise Error("claim requires 'nameplate'")
allow_allocate = True
if "allocate" in msg:
allow_allocate = msg["allocate"]

if self._did_claim:
raise Error("only one claim per connection")
self._did_claim = True
nameplate_id = msg["nameplate"]
assert isinstance(nameplate_id, type("")), type(nameplate_id)
self._nameplate_id = nameplate_id
try:
mailbox_id = self._app.claim_nameplate(nameplate_id, self._side,
server_rx)
server_rx, allow_allocate)
except CrowdedError:
raise Error("crowded")
except ReclaimedError:
raise Error("reclaimed")
except UnknownNameplateError:
raise Error("unallocated")
self._did_claim = True
self.send("claimed", mailbox=mailbox_id)

def handle_release(self, msg, server_rx):
Expand Down
38 changes: 27 additions & 11 deletions src/wormhole_mailbox_server/test/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ def test_defaults(self):
self.assertEqual(o, {"port": PORT,
"channel-db": "relay.sqlite",
"disallow-list": 0,
"allow-list": True,
"allow-list": False,
"advertise-version": None,
"signal-error": None,
"usage-db": None,
Expand All @@ -28,7 +28,7 @@ def test_advertise_version(self):
self.assertEqual(o, {"port": PORT,
"channel-db": "relay.sqlite",
"disallow-list": 0,
"allow-list": True,
"allow-list": False,
"advertise-version": "1.0",
"signal-error": None,
"usage-db": None,
Expand All @@ -44,7 +44,7 @@ def test_blur(self):
self.assertEqual(o, {"port": PORT,
"channel-db": "relay.sqlite",
"disallow-list": 0,
"allow-list": True,
"allow-list": False,
"advertise-version": None,
"signal-error": None,
"usage-db": None,
Expand All @@ -60,7 +60,7 @@ def test_channel_db(self):
self.assertEqual(o, {"port": PORT,
"channel-db": "other.sqlite",
"disallow-list": 0,
"allow-list": True,
"allow-list": False,
"advertise-version": None,
"signal-error": None,
"usage-db": None,
Expand All @@ -86,13 +86,29 @@ def test_disallow_list(self):
"websocket-protocol-options": [],
})

def test_allow_list(self):
o = server_tap.Options()
o.parseOptions(["--allow-list"])
self.assertEqual(o, {"port": PORT,
"channel-db": "relay.sqlite",
"disallow-list": 0,
"allow-list": True,
"advertise-version": None,
"signal-error": None,
"usage-db": None,
"blur-usage": None,
"motd": None,
"log-fd": None,
"websocket-protocol-options": [],
})

def test_log_fd(self):
o = server_tap.Options()
o.parseOptions(["--log-fd=5"])
self.assertEqual(o, {"port": PORT,
"channel-db": "relay.sqlite",
"disallow-list": 0,
"allow-list": True,
"allow-list": False,
"advertise-version": None,
"signal-error": None,
"usage-db": None,
Expand All @@ -108,7 +124,7 @@ def test_port(self):
self.assertEqual(o, {"port": "tcp:5555",
"channel-db": "relay.sqlite",
"disallow-list": 0,
"allow-list": True,
"allow-list": False,
"advertise-version": None,
"signal-error": None,
"usage-db": None,
Expand All @@ -123,7 +139,7 @@ def test_port(self):
self.assertEqual(o, {"port": "tcp:5555",
"channel-db": "relay.sqlite",
"disallow-list": 0,
"allow-list": True,
"allow-list": False,
"advertise-version": None,
"signal-error": None,
"usage-db": None,
Expand All @@ -139,7 +155,7 @@ def test_signal_error(self):
self.assertEqual(o, {"port": PORT,
"channel-db": "relay.sqlite",
"disallow-list": 0,
"allow-list": True,
"allow-list": False,
"advertise-version": None,
"signal-error": "ohnoes",
"usage-db": None,
Expand All @@ -155,7 +171,7 @@ def test_usage_db(self):
self.assertEqual(o, {"port": PORT,
"channel-db": "relay.sqlite",
"disallow-list": 0,
"allow-list": True,
"allow-list": False,
"advertise-version": None,
"signal-error": None,
"usage-db": "usage.sqlite",
Expand All @@ -171,7 +187,7 @@ def test_websocket_protocol_option_1(self):
self.assertEqual(o, {"port": PORT,
"channel-db": "relay.sqlite",
"disallow-list": 0,
"allow-list": True,
"allow-list": False,
"advertise-version": None,
"signal-error": None,
"usage-db": None,
Expand All @@ -189,7 +205,7 @@ def test_websocket_protocol_option_2(self):
self.assertEqual(o, {"port": PORT,
"channel-db": "relay.sqlite",
"disallow-list": 0,
"allow-list": True,
"allow-list": False,
"advertise-version": None,
"signal-error": None,
"usage-db": None,
Expand Down
12 changes: 7 additions & 5 deletions src/wormhole_mailbox_server/test/test_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ def test_nameplate_allocation(self):
nids = set()
# this takes a second, and claims all the short-numbered nameplates
def add():
nameplate_id = app.allocate_nameplate("side1", 0)
nameplate_id, mailbox_id = app.allocate_nameplate("side1", 0)
self.assertEqual(type(nameplate_id), type(""))
self.assertEqual(type(mailbox_id), type(""))
nid = int(nameplate_id)
nids.add(nid)
for i in range(9): add()
Expand Down Expand Up @@ -54,11 +55,11 @@ def _get_nameplate_ids():

def test_nameplate(self):
app = self._server.get_app("appid")
name = app.allocate_nameplate("side1", 0)
name, mailbox = app.allocate_nameplate("side1", 0)
self.assertEqual(type(name), type(""))
nid = int(name)
self.assert_(0 < nid < 10, nid)
self.assertEqual(app.get_nameplate_ids(), set([name]))
self.assertEqual(app._get_nameplate_ids(), set([name]))
# allocate also does a claim
np_row, side_rows = self._nameplate(app, name)
self.assertEqual(len(side_rows), 1)
Expand Down Expand Up @@ -275,8 +276,9 @@ def test_early_close(self):
other side joins.
"""
app = self._server.get_app("appid")
name = app.allocate_nameplate("side1", 42)
name, allocate_mailbox = app.allocate_nameplate("side1", 42)
mbox = app.claim_nameplate(name, "side1", 0)
self.assertEqual(mbox, allocate_mailbox)
m = app.open_mailbox(mbox, "side1", 0)
m.close("side1", "mood", 1)

Expand Down Expand Up @@ -511,7 +513,7 @@ def test_nameplate_disallowed(self):
def test_nameplate_allowed(self):
db = create_channel_db(":memory:")
a = AppNamespace(db, None, None, False, "some_app_id", True)
np = a.allocate_nameplate("side1", "321")
np, mb = a.allocate_nameplate("side1", "321")
self.assertEqual(set([np]), a.get_nameplate_ids())

def test_blur(self):
Expand Down
4 changes: 2 additions & 2 deletions src/wormhole_mailbox_server/test/test_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def test_defaults(self):
s = server_tap.makeService(o)
self.assertEqual(ccdb.mock_calls, [mock.call("relay.sqlite")])
self.assertEqual(ccub.mock_calls, [mock.call(None)])
self.assertEqual(ms.mock_calls, [mock.call(cdb, allow_list=True,
self.assertEqual(ms.mock_calls, [mock.call(cdb, allow_list=False,
advertise_version=None,
signal_error=None,
welcome_motd=None,
Expand All @@ -46,7 +46,7 @@ def test_log_fd(self):
return_value=fd) as f:
server_tap.makeService(o)
self.assertEqual(f.mock_calls, [mock.call(99, "w")])
self.assertEqual(ms.mock_calls, [mock.call(cdb, allow_list=True,
self.assertEqual(ms.mock_calls, [mock.call(cdb, allow_list=False,
advertise_version=None,
signal_error=None,
welcome_motd=None,
Expand Down
10 changes: 8 additions & 2 deletions src/wormhole_mailbox_server/test/test_web.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ def setUp(self):
self._usage_db = usage_db = create_or_upgrade_usage_db(":memory:")
yield self._setup_relay(do_listen=True,
advertise_version="advertised.version",
usage_db=usage_db)
usage_db=usage_db,
allow_list=True)

def tearDown(self):
for c in self._clients:
Expand Down Expand Up @@ -222,7 +223,7 @@ def test_list(self):
self.assertEqual(m["nameplates"], [])

app = self._server.get_app("appid")
nameplate_id1 = app.allocate_nameplate("side", 0)
nameplate_id1, mailbox_id1 = app.allocate_nameplate("side", 0)
app.claim_nameplate("np2", "side", 0)

c1.send("list")
Expand Down Expand Up @@ -280,6 +281,11 @@ def test_claim(self):
self.assertEqual(err["type"], "error")
self.assertEqual(err["error"], "claim requires 'nameplate'")

c1.send("claim", nameplate="np1", allocate=False) # not allocated
err = yield c1.next_non_ack()
self.assertEqual(err["type"], "error")
self.assertEqual(err["error"], "unallocated")

c1.send("claim", nameplate="np1")
m = yield c1.next_non_ack()
self.assertEqual(m["type"], "claimed")
Expand Down