Skip to content

Redesigned the internal architecutre and reviewed APIs #77

Closed
wants to merge 29 commits into from

4 participants

@pokehanai
  • Redesigned the internal architecutre and reviewed APIs. Mostly for CPU/memory performance, stability and reliability.
  • Introduced new NotificationBuckcet object, which enables client to send multiple notifications within a single transimission for traffic efficient delivery.
  • Eech time the Connection object try to rebuild its connection when the transmission ends up. This behavior has been required for preventing unexpected EPIPE error, which can be observed when data size in t\ otal larger than 8192 bytes is sent via a connection.
  • Removed Notification.device and Connection.sendNotification.
  • Removed sent notification caching feature and related options cacheLength and autoAdjustCache.
  • Removed Device object and dropped device.js.
  • Removed options.errorCallback from Connection service options.
  • Added Connection.addNotification.
  • Added Connection.flush.
  • Added options bucketLength and notificationWaitingTime.
  • Added Connection#notificationWaitingTimer to flush notifications automatically and periodically when the bucket is not full.
  • Added Connection.ready and Connection.rejected events.
  • Replaced the second argument of Connection#raiseError event from notification to deviceToken. Since the Connection doesn't store plain notification objects no more.
  • Added tests, work on busterjs.
  • Introduced event system to Feedback. Please see the section above for more details.
  • Added options.tokenType to Feedback service options.
  • Removed options.feedback and options.errorCallback from Feedback service options.
  • Removed options.batchFeedback and dropped related function; it is not library's task but client's.
pokehanai added some commits Jan 8, 2013
@pokehanai pokehanai Changed notification architecutre; Added batch send, reduce EPIPE error.
For end users:
- Refreshed architecture; now it sends multiple notification in single
  transmission. And reduced EPIPE errors you may had seen.
- Removed `Notification#device` and removed `Connection#sendNotification`.
- Added `Connection#addNotification`.
- Added `Connection#flush`.
- Added `Connection#broadcast`. Which enables you to broadcast same
  notification content to many devices in a memory efficient way.
- Added option `bucketLength` and `notificationWaitingTime`.

For package developers:
- Added feature of notification bucket, this enables you to send
  multiple notifications in a single transimission for traffic
  efficient delivery. Also it has come to prevent unexpectable EPIPE
  error; the error can be observed when some amount of data has been
  sent via a connection, > 8192 bytes. Now the connection reopen
  itself every time after the bucket went out.
- Added connection state constants and variable and make
  `Connection#deferredConnection` exists always. This is required to
  determine connection available state in `Connection#run`.
- Added `Connection#notificationWaitingTimer` to flush notifications
  automatically and periodically when the bucket is not full.
- Removed `notification` argument from `Connection#raiseError`
  event. Since the effort won't reward to retrive notification from
  the bucket.
- Disabled `Connection#disconnected` event.
- Added tests, work with busterjs.
2743888
@pokehanai pokehanai Updated .gitignore. 811b2f6
@pokehanai pokehanai lint. 0ff91b4
@pokehanai pokehanai Changed mothod name Notification#toBinaryTemplate to Notification#get…
…CompiledNotification.

Then renamed related variable names.
- template -> compiledNotification.
- notificationOrTemplate -> notification.
Also added `isCompiled` member to the compiled notification.
5881936
@pokehanai pokehanai Reduce overhead in Connection#showState, a debug purpose method. a964c67
@pokehanai pokehanai Enable to callback of broadcast to return any false value, e.g. null.
Previously it was only allowed to return `false` to stop the cycle.
23d47a5
@pokehanai pokehanai Added Connection#bulksend.
Useful to send a variety of notification contents in pull-contents-model.
e2e136d
@pokehanai pokehanai Removed obsolete options `cacheLength` and `autoAdjustCache`. 838b50d
@pokehanai pokehanai Enabled Connection#disconnected event again. ac2e35c
@pokehanai pokehanai Changed mothod name Notification#getCompiledNotification to Notificat…
…ion#compile
081ccb4
@pokehanai pokehanai Fixed a typo. 2fac092
@pokehanai pokehanai Updated README. dbd0999
@pokehanai pokehanai Update README.md d2bbe38
@pokehanai pokehanai Update README.md 819a223
@pokehanai pokehanai Added `scripts` section in package.js. 8c101a5
@pokehanai pokehanai Removed `Connection#broadcast` and `Connection#bulksend`.
It wouldn't match with async architecture.
Possibly it could adapt double dispatching model(the callback accepts
another callback, to submit a fetched content), but it seemed to cause
many bugs in end users application, so left it just as an idea.
1ff3ef0
@pokehanai pokehanai Fixed notificationWaitingTimer handling issue. 8244cd3
@pokehanai pokehanai Added Connection#countNotification and Connection#hasNotification. b1838d8
@pokehanai pokehanai lint. 0ed85ca
@pokehanai pokehanai Updated README f62c51a
@pokehanai pokehanai Merge branch 'master' of https://github.com/michaelvillar/node-apn in…
…to develop
32c2b8e
@pokehanai pokehanai Now Connection#transmissionError and errorCallback receieve deviceToken. b47d3cd
@pokehanai pokehanai Changed event name `bucketAvailable` to `ready`. 03d0990
@pokehanai pokehanai Improved `Feedback` performance, modified some API.
- Inherited `EventEmitter` object.
- Added some events.
- device token in `feedback` and `batchFeedback` events is a string instead of `Device` object. In this way it uses less CPU and memory.
- Removed `options.feedback` and `options.errorCallback`.
0bea496
@pokehanai pokehanai Removed options.errorCallback from Connect object options.
And updated README.
eb26b5b
@pokehanai pokehanai Removed options.batchFeedback and related function.
It is not library's task but client's.
With events found in the current implementation, client can very same
thing by it's own hand.
1f0e50f
@pokehanai pokehanai Added `options.tokenType` to `Feedback` service options.
A string value, either "string" or "binary". Specifies type of the
`device token` argument of the `feedback` event.

If it is "string", `device token` will be a hex string.
If it is "binary", `device token` will be a `Buffer` object.
55406f3
@pokehanai pokehanai Updated README and modify method comment of `Connection.connect`. ef1aea8
@pokehanai pokehanai Merge branch 'develop' ef4c7c3
@argon
Owner
argon commented Jan 18, 2013

I will not be accepting this pull request in full at this time. I do not agree with some of your design choices. You have given me some interesting ideas and I may choose to use some of your commits but I see some missed opportunities in the work you have done.

I will leave this request open for now, so that others can choose to evaluate your work and if they wish to share their opinion we can discuss it.

@pokehanai

I understand.
And I will appreciate you and other developers' opinion.

@mavrick
mavrick commented Jan 25, 2013

Can we leave in the original end points please but add in the ability to have a callback in each so I may proxy a function upon completion of sendNotification etc?

@pokehanai

We may be able to add events of 2.x to 1.x somehow.
But while I have not find any benefit with the Device object, I think it just have brought costs for CPU/memory/speed, 2.x shouldn't have it. So I think it is required to break the backward compatibility for better server performance.

@mgcrea
mgcrea commented Feb 8, 2013

Would love to see this merged,

@argon I think you should merge it (though not tag it yet) & build from there towards something you like.
The current code has several flaws (feedback not having events, etc.) that this PR could fix.

Will try this code in the coming days.

@argon
Owner
argon commented Feb 8, 2013

I appreciate your comments. I agree with some of the decisions but I do not like the sentinel solution at all. It is not good behaviour to create and destroy so many connections. I have some work in the pipeline which will address some of the concerns, in particular the feedback system events which I will push this weekend.

@argon
Owner
argon commented Feb 9, 2013

In addition to the feedback system eventing which I have implemented (available on 'develop' branch) I am going to be deprecating the device object and adding some new API.

@argon argon closed this Feb 9, 2013
@argon
Owner
argon commented Apr 10, 2013

A quick update to the sentinel idea that has been implemented in this code, the following quote from TN2265 under "Problems Connecting to the Push Service"

"Another possibility is that you've connected too many times to APNs and further connections have been temporarily blocked. As is documented in the Local and Push Notification Programming Guide, developers are expected to open a connection and leave it open. If a connection is opened and closed repeatedly, APNs will treat it as a denial of service attack and block connections for a period of time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.