Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

Feature/udp #111

Merged
merged 24 commits into from
Sep 14, 2015
Merged

Feature/udp #111

merged 24 commits into from
Sep 14, 2015

Conversation

jrconlin
Copy link
Member

Fixes #106.

  • Add UDP Wake support for SimplePush using router
    ** Adds timeout to boot connections that specify UDP wake.
  • Fix some flake8 issues
  • convert tests that use ....assert_called() to use eq_(....called, True) to fix travis incompatibilities
  • Fix missing labels for some args.
  • convert some worrisome " is " tests to " == "
  • bumped pyasn to 0.1.8 to fix install issue.

@bbangert @kitcambridge r?

jrconlin and others added 7 commits July 6, 2015 16:40
* Added label to APNS args
* TODO: Tests
* Added first go on idle timeouts for websocket connections
* TODO: is there a path for the UDP wake server or just go to that
  host/port?
* start testing
Rewrote UDP as a wake function for SimplePush. There's potentially
a bit of wake abstraction that could be made in the future, but that's
best done when there's a future requirement.
* Fixing some formatting
* added router_conf for simplepush (since it now has stuff, sorta)
* Added edge case for check_idle
* fix to correct timeout to be sec not millisec
* fix to some improper is usage
* restore some code from test tweaks.
* convert assert_called to eq_(...called, True)
* removed websocket router init test
** Because we don't do routing registrations over websockets.
** Why, yes, I had forgotten that and spent an hour trying to
   wire it back in.
@@ -247,7 +247,7 @@ def register_user(self, data):
for key, value in result["Attributes"].items():
try:
r[key] = self.table._dynamizer.decode(value)
except AttributeError:
except (TypeError, AttributeError):
Copy link

Choose a reason for hiding this comment

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

What does the TypeError catch?

Copy link
Member Author

Choose a reason for hiding this comment

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

If value is not proper javascript, it throws a TypeError. If table or _dynamizer aren't initialized, they throw Attribute error. In either case, just take the value as specified and don't be clever.

* change idle_timeout to udp_timeout
* add udp_server as arg, pass UDP wake args as urlencoded dict
@@ -76,7 +76,7 @@ def stdout(message):
else:
sys.stdout.write(" %s\n" % message['message'])
return
"""
# """
Copy link
Member

Choose a reason for hiding this comment

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

This seems unintentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, sorta intentional. It lets me activate the "human readable logging" by adding a # to the start of line 73 instead of to both lines. The triple quote stops on that line regardless of the starting hash.

@bbangert
Copy link
Member

I'd really like to see the wakeup split out of the router/websocket into its own small object with a few methods that implements a Wake interface (and hooked onto the completion of routing). I won't be surprised if in the future we may end up doing something similar to what Google does, where we actually have all active mobile devices connect directly to us, but when 'gone idle', they drop, and we wake them with GCM/APNS/etc. In which case we can easily grow several wakeup methods, and having each one be a custom one-off in this style will impair us.

@jrconlin
Copy link
Member Author

Fair. I considered this a "one off" mostly because that's all we at at the moment, and the alternate bridge mechanisms handled their own idle periods. Having a wake interface makes sense. Do we expect there to be other side event interfaces (Has data/ Priority/ etc.)?

@bbangert
Copy link
Member

Well, the reason I could see copying Google's approach, is that we might find GCM/APNS lacking in some way for optimal message delivery/latency, so FF on mobile when its in-use would actually hold open a connection directly to us, but drop it as soon as the app is backgrounded/killed.

* Fix typo in default collapse key for gcm
* make wake references more generic
Looks like i need to figure out what's goofy with my vim config
@jrconlin
Copy link
Member Author

Note: this may require refactoring if PR #115

Conflicts:
	requirements.txt
…to feature/udp

Conflicts:
	autopush/tests/test_endpoint.py
	autopush/tests/test_router.py
	autopush/tests/test_websocket.py
@jrconlin jrconlin self-assigned this Jul 27, 2015
@@ -70,6 +78,12 @@ def route_notification(self, notification, uaid_data):
# Determine if they're connected at the moment
node_id = uaid_data.get("node_id")
uaid = uaid_data["uaid"]
self.udp = None
try:
self.udp = json.loads(uaid_data["udp"])
Copy link
Member

Choose a reason for hiding this comment

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

The common path for 99% of clients is going to be no UDP, exceptions are expensive in Python so we're essentially slow-pathing the common case by adding an exception to it. A 'if udp in uaid_data' test would be fast.

Copy link
Member

Choose a reason for hiding this comment

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

Also, uaid_data is a dict-like item out of DynamoDB. DynamoDB items can store structured nested dicts, so in theory we don't need to JSON encode it on the way in/out like this.

@bbangert bbangert modified the milestones: 1.7.0, 1.6.0 Sep 14, 2015
@@ -138,6 +138,9 @@ Features
* Add Sphinx docs with ReadTheDocs publishing. Issue #98.
This change also includes a slight Metrics refactoring with a IMetrics
interface, and renames MetricSink -> SinkMetrics for naming consistency.
* Add UDP Wake support. Issue #106
Some devices which use SimplePush routing offer a feature to wake on
a carrier provided UDP ping. See issue #106 for details.
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be moved to a 1.7.0 section now. Sorry bout that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Heh, tempted to move it to ∞.0.2 at this point.

@bbangert
Copy link
Member

r+ pending deferToThread change.

if self.udp is not None and "server" in self.conf:
# Attempt to send off the UDP wake request.
try:
yield requests.post(
Copy link
Member

Choose a reason for hiding this comment

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

Er, I dunno why this works in the tests. yield can only be done on results that are deferred, requests.post is a blocking call that doesn't return a deferred. It needs a deferToThread in front.

@codecov-io
Copy link

Current coverage is 99.95%

Merging #111 into master will not affect coverage as of a51ef81

@@            master    #111   diff @@
======================================
  Files           27      27       
  Stmts         4934    4992    +58
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           4932    4990    +58
  Partial          0       0       
  Missed           2       2       

Review entire Coverage Diff as of a51ef81

Powered by Codecov. Updated on successful CI builds.

@bbangert
Copy link
Member

r+

bbangert added a commit that referenced this pull request Sep 14, 2015
@bbangert bbangert merged commit 1e406c0 into master Sep 14, 2015
@bbangert bbangert deleted the feature/udp branch September 14, 2015 23:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a UDP wake-up bridge
3 participants