Breaking: Full support for websocket-based notifications and eol'ed http notifications #279
Conversation
gip
commented
Jun 29, 2016
gip
commented
- Removed http notifications and associated db tables / logic
- Client is able to ask for notification being resend
- Associated PR: Switching to websocket notifications five-bells-receiver#3
Current coverage is 91.41%@@ master #279 diff @@
==========================================
Files 52 43 -9
Lines 1661 1328 -333
Methods 230 182 -48
Messages 0 0
Branches 278 226 -52
==========================================
- Hits 1528 1214 -314
+ Misses 133 114 -19
Partials 0 0
|
| @@ -177,6 +179,13 @@ function * subscribeTransfers () { | |||
| }) | |||
|
|
|||
| this.websocket.on('close', close) | |||
| // The client can request a notification for a given transfer id | |||
There was a problem hiding this comment.
@MatthewPhinney I'm not ware of us having any documentation for the websocket protocol as of yet. Let me know if you happen to know.
gip
force-pushed
the
ws_notifications
branch
3 times, most recently
from
1f28f03
to
5d31e6c
Compare
| @@ -481,11 +473,27 @@ function * insertTransfers (externalTransfers) { | |||
| converters.convertToInternalTransfer)) | |||
| } | |||
|
|
|||
| function * resendNotification (transferId) { | |||
There was a problem hiding this comment.
I think you want some kind of check here to ensure the party requesting the notification has privileges to view this transfer ... although it doesn't look like the get transfer endpoint has any sort of authentication either.
There was a problem hiding this comment.
Good points, but actually I don't think it's a problem because:
- When a request is sent, a lookup is made in the database. If the transfer exists, the notification will first be emitted using the event name
transfer-{$ACCOUNT}. See https://github.com/interledger/five-bells-ledger/blob/ws_notifications/src/lib/notificationBroadcasterWebsocket.js#L52 - So the notification will only be sent to the ws connections that listens to that account, no worries there
That being said, if a client sends a request with a transfer id which correspond to a transfer that doesn't belong to that account:
- A notification will be sent to the clients listening for notifications on that account, even though they have not requested a new notification
- I don't think that is a problem as this is very unlikely to happen. The probability of collision for generated UUIDs is probably lower than the risk of being reincarnated as an olive in a next life
There is an issue with this implementation though. If 2 (or more) clients are listening for notifications for a given account and only one of the client requests a new notification for a given transfer. Then all the clients will receive the notification, not only the requesting one. We could either:
- Change this behaviour (that would take some work)
- Add to the specs that notification may be sent more than once. Clients would therefore need to be idempotent
There was a problem hiding this comment.
Yeah, that makes sense. I think we can expect clients to be robust enough to handle duplicate notifications, should they occur.
gip
force-pushed
the
ws_notifications
branch
4 times, most recently
from
3d39fd7
to
5d95bf8
Compare
|
Agree on taking out the REST hooks. This will significantly improve ledger performance. Not sure why the resend functionality is being added. Instead of requesting a resend, what's wrong with simply I'm assuming that this design was chosen in order to avoid duplicating the code that currently exists in the websocket notification handler. May I suggest simply refactoring the app such that the notification handler is a reusable function which can be called both from the notification handler and when manually requesting the transfer from the ledger. This will be simpler, more performant and removes the need for the resend feature in the ledger. |
…ttp notifications
|
The resend notification feature may be better in term if performance and stability for the ledger - doing things asynchronously usually brings better performance in large distributed systems. The ledger would have better control over what it being executed. Plus there would be a unique channel for a client to get notifications. I don't think this (huge) PR should be delayed by this discussion however. In the interest of speed, I have removed the resend logic for now. I'll do a new PR once we've discussed further and find an agreement. Can you please review @justmoon. |
|
LGTM Improves performance and I'm happy to see this much dead code removed. 👍 |
