FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

destroyable EventStreams; closes #302 by boneskull · Pull Request #304 · box/box-node-sdk · GitHub

  1. box
  2. box-node-sdk

Repository navigation

destroyable EventStreams; closes #302 - #304

Merged
mattwiller merged 2 commits into
box:masterfrom
boneskull:destroy-event-stream
Aug 27, 2018
ConversationCommits (2)ChecksFiles changed
Merged

destroyable EventStreams; closes #302#304
mattwiller merged 2 commits into
box:masterfrom
boneskull:destroy-event-stream

Conversation

boneskull commented Aug 7, 2018

Copy link
Copy Markdown
Contributor
  • add reference to long-polling timer on class
  • add EventStream method _destroy(), which clears any existing
    long-polling timer, and deletes it. Destroys stream if not already
    destroyed. Implements Readable#_destroy in Node.js v8.x and newer.
  • add EventStream method abort(), an alias for _destroy(), which
    is not called via EventStream#destroy() in pre-Node.js-v8.x. Any
    version can use this method; _destroy() is private.
  • add checks for destroyed stream state which effectively abort
    various tasks in EventStream
  • consolidate long-polling retry logic into its own private method,
    EventStream#retryLongPoll().
  • add tests for logic changes
  • add note in docs

It'd be helpful to have an integration test (without use of Sinon's fake timers) which asserts a destroyed stream doesn't still have tasks in the event loop. If it does, such a test should hang--ostensibly due to the infinite recursion--because of Mocha's v4.x+ behavior.

Originally, I discovered this problem when writing some test code which consumes an EventStream. I could determine that this implementation was correct, because my test code was no longer hanging indefinitely. 😄

boxcla commented Aug 7, 2018

Copy link
Copy Markdown

Verified that @boneskull has signed the CLA. Thanks for the pull request!

coveralls commented Aug 7, 2018 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Pull Request Test Coverage Report for Build 1364

  • 18 of 18 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 97.346%
Totals
Change from base Build 1363: 0.02%
Covered Lines: 2145
Relevant Lines: 2173
💛 - Coveralls

boneskull force-pushed the destroy-event-stream branch from a353ac5 to 8e1168d Compare August 7, 2018 21:04

mattwiller commented Aug 7, 2018

Copy link
Copy Markdown

@boneskull Overall the implementation looks good to me, but it looks like tests aren't passing on Node.js versions before 8.x — you'll need to ensure your code works correctly for versions all the way back to Node.js 4.0.0

boneskull force-pushed the destroy-event-stream branch from 8e1168d to 48c8aa4 Compare August 7, 2018 22:44

boneskull commented Aug 7, 2018

Copy link
Copy Markdown
Contributor Author

The coverage % decreased because this PR adds more covered LoC to the project. I don't love how Coveralls calculates that...

boneskull commented Aug 7, 2018

Copy link
Copy Markdown
Contributor Author

@mattwiller The failures have been fixed on Travis-CI

mattwiller reviewed Aug 7, 2018
View reviewed changes
Comment thread lib/event-stream.js Outdated
if (!this.destroyed) {
/* istanbul ignore else */
if (typeof this.destroy === 'function') {
this.destroy();

mattwiller Aug 7, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The Node.js Stream docs say that _destroy() is called by destroy() — why call back into destroy() here?

boneskull Aug 8, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Because destroy was not added until v8 of Node.

boneskull Aug 8, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

(so if you called abort you might as well have called destroy if it exists. this is moot if I just conditionally add the method)

Comment thread lib/event-stream.js Outdated
* @returns {void}
* @public
*/
EventStream.prototype.abort = EventStream.prototype._destroy;

mattwiller Aug 7, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's a bit undesirable to have two different methods to call depending on which version of Node.js the user is running. Is it possible to check if EventStream.prototype.destroy already exists and conditionally add/polyfill it if it doesn't?

boneskull Aug 8, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yeah, we could do that.

boneskull force-pushed the destroy-event-stream branch 2 times, most recently from 4334ae6 to e128da5 Compare August 8, 2018 21:26

boneskull commented Aug 8, 2018

Copy link
Copy Markdown
Contributor Author

@mattwiller Build needs retry; Node.js v9 didn't seem to start.

mattwiller suggested changes Aug 9, 2018
View reviewed changes
Comment thread lib/event-stream.js Outdated
};

// backwards-compat for Node.js pre-v8.0.0
if (!(typeof Readable.destroy === 'function')) {

mattwiller Aug 9, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You'll need to check against Readable.prototype.destroy here — this condition as written will always include the polyfill. Also, using !== might be more readable than !(... === ...)

boneskull Aug 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

my bad

boneskull force-pushed the destroy-event-stream branch from e128da5 to e3f1622 Compare August 9, 2018 21:05
destroyable EventStreams; closes box#302
63fc449 
- add reference to long-polling timer on class
- add `EventStream` method `_destroy()`, which clears any existing
  long-polling timer, and deletes it. Destroys stream if not already
  destroyed.  Implements `Readable#_destroy` in Node.js v8.x and newer.
- add polyfill for `Readable#destroy`, which isn't present
  pre-Node.js-v8.x
- add checks for `destroyed` stream state which effectively abort
  various tasks in `EventStream`
- consolidate long-polling retry logic into its own private method,
  `EventStream#retryLongPoll()`.
- add tests for logic changes
- add note in docs
boneskull force-pushed the destroy-event-stream branch from e3f1622 to 63fc449 Compare August 9, 2018 21:45

boneskull commented Aug 9, 2018

Copy link
Copy Markdown
Contributor Author

@mattwiller addressed your latest comments

boneskull mentioned this pull request Aug 9, 2018
Open

boneskull commented Aug 27, 2018

Copy link
Copy Markdown
Contributor Author

@mattwiller Do you need anything else from me here?

mattwiller merged commit 15eb243 into box:master Aug 27, 2018
boneskull deleted the destroy-event-stream branch August 27, 2018 22:54

boneskull commented Sep 7, 2018

Copy link
Copy Markdown
Contributor Author

@mattwiller Possible to release this soon?

mattwiller commented Sep 12, 2018

Copy link
Copy Markdown

@boneskull We're looking to push out a release later this week!

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
mattwiller mattwiller approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL