Skip to content

Conversation

boneskull
Copy link
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
Copy link

boxcla commented Aug 7, 2018

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

@coveralls
Copy link
Collaborator

coveralls commented Aug 7, 2018

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 Coverage Status
Change from base Build 1363: 0.02%
Covered Lines: 2145
Relevant Lines: 2173

💛 - Coveralls

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

@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 boneskull force-pushed the destroy-event-stream branch from 8e1168d to 48c8aa4 Compare August 7, 2018 22:44
@boneskull
Copy link
Contributor Author

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

@boneskull
Copy link
Contributor Author

@mattwiller The failures have been fixed on Travis-CI

if (!this.destroyed) {
/* istanbul ignore else */
if (typeof this.destroy === 'function') {
this.destroy();

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because destroy was not added until v8 of Node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

* @returns {void}
* @public
*/
EventStream.prototype.abort = EventStream.prototype._destroy;

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, we could do that.

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

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

};

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

Choose a reason for hiding this comment

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

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 !(... === ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad

@boneskull boneskull force-pushed the destroy-event-stream branch from e128da5 to e3f1622 Compare August 9, 2018 21:05
- 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 boneskull force-pushed the destroy-event-stream branch from e3f1622 to 63fc449 Compare August 9, 2018 21:45
@boneskull
Copy link
Contributor Author

@mattwiller addressed your latest comments

@boneskull
Copy link
Contributor Author

@mattwiller Do you need anything else from me here?

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

@mattwiller Possible to release this soon?

@mattwiller
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants