This repository has been archived by the owner. It is now read-only.

/node-v0.x-archive

WIP: Verify core events and patterns #13862

Closed
wants to merge 7 commits into
base: v0.12
from

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
5 participants
@tjfontaine @jasnell @trevnorris @Nodejs-Jenkins @orangemocha
@tjfontaine

tjfontaine commented Mar 24, 2015

Before we can start stamping out what it means to be "compatible" with Node.js, we have to start by writing down how we think Node.js works today, and then verifying that scenario. (cc @dshaw @trevnorris @jasnell @mdawsonibm As we've all had conversations about API compatibility in the past)

Since we're event driven the key abstraction/data structure that impacts all of Node.js is EventEmitter. While we have spent the time to get that useful and fast, we haven't spent as much time making sure how Node.js presents its interfaces via the EventEmitter are meeting our expectations.

There are many places we can improve Node.js, but one of our friction points today (internally and externally) involves around what events will happen, when, and in what order.

This branch/PR adds the notion of a StrictEE implementation of EventEmitter that provides facilities to describe your events.

Instead of inheriting from plain old EE you inherit from StrictEE, and then when constructing you pass in an object which defines the events and the orders that can be passed in.

I myself am not convinced if this should be used in production, there is definitely plenty of low hanging fruit for improving its performance (it should create way fewer object literals). But really, I think (unless the user has asked for "strict mode") normal operating procedure should be that most of the strict checks are noops.

Instead, I think this is critical while running our test suite, and verifying our software (and our downstream users code).

A definition of a rule looks like this currently:

{
  myEvent: {
    minCount: 1,
    maxCount: 2,
    after: ['event1', 'event2', 'event3'],
    notAfter: ['event4', 'event5'],
  },
}

and running our test suite you can grep the output and get something like this:

   2 #AssertionError: Event "close" must fire after at least one of ["error","end","finish","destroy","aborted"], fired []
   2 #AssertionError: Event "close" must fire after at least one of ["error","upgrade","aborted","finish","end"], fired ["socket","drain","response"]
   2 #AssertionError: Event "close" must fire after at least one of ["listening","error"], fired []
   2 #AssertionError: Event "listening" fired 2 times, more than 1
   2 #AssertionError: Event "unpipe" must **not** fire after "error", fired ["pipe","error"]

It's a very simple description, after is better defined: "after any", and notAfter is "not after any", and there is a mechanism for counting the times which an event has fired. We probably want the concept of afterAll.

In practice minCount is not used anywhere, and its overhead and potential to leak (in its current implementation) should discourage use of this without also growing some concept of a finalization step that could be used to verify this instead of the current global data structure.

You can also use obj.strictEERegister to extend rule definitions in child classes.

There probably should also be an obj.strictEEReset mode that resets the "state machine" for reuse (this already found an issue in net.connect re-usage).

This is currently based on v0.12 because I found it helpful while debugging other current bugs. In its current state it's a public API addition that wouldn't be suitable for v0.12.1, but I do think it's useful to finish this work on the current stable branch and rename APIs and any additional events such that we can continue to squash bugs.

Other obvious oversights include missing tests and documentation.

I'm publishing this PR as I'm looking for help from someone in the community who wants to come and help complete this. As a result, you'll end up with a lot of more knowledge about how Node.js actually works. I myself learned quite a bit bootstrapping into this. For everyone else, we'll have described and along the way documented quite a bit of how Node.js works, which will make it easier for the next person to come and contribute a fix.

@tjfontaine tjfontaine added the in progress label Mar 24, 2015

@Nodejs-Jenkins Nodejs-Jenkins added pr v0.12 labels Mar 24, 2015

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 24, 2015

Member

I recently sat down with Bert and Michael Dawson and had a conversation
around improvements around the internal APIs. That conversation and this
one pair nicely. I'll help where I can. Either later on this evening or
tomorrow I'll dig in a bit deeper in response to your write up here.
On Mar 24, 2015 3:10 PM, "Timothy J Fontaine" notifications@github.com
wrote:

Before we can start stamping out what it means to be "compatible" with
Node.js, we have to start by writing down how we think Node.js works today,
and then verifying that scenario. (cc @dshaw https://github.com/dshaw
@trevnorris https://github.com/trevnorris @jasnell
https://github.com/jasnell @mdawsonibm https://github.com/mdawsonibm
As we've all had conversations about API compatibility in the past)

Since we're event driven the key abstraction/data structure that impacts
all of Node.js is EventEmitter. While we have spent the time to get that
useful and fast, we haven't spent as much time making sure how Node.js
presents its interfaces via the EventEmitter are meeting our expectations.

There are many places we can improve Node.js, but one of our friction
points today (internally and externally) involves around what events will
happen, when, and in what order.

This branch/PR adds the notion of a StrictEE implementation of
EventEmitter that provides facilities to describe your events.

Instead of inheriting from plain old EE you inherit from StrictEE, and
then when constructing you pass in an object which defines the events and
the orders that can be passed in.

I myself am not convinced if this should be used in production, there is
definitely plenty of low hanging fruit for improving its performance (it
should create way fewer object literals). But really, I think (unless the
user has asked for "strict mode") normal operating procedure should be that
most of the strict checks are noops.

Instead, I think this is critical while running our test suite, and
verifying our software (and our downstream users code).

A definition of a rule looks like this currently:

{
myEvent: {
minCount: 1,
maxCount: 2,
after: ['event1', 'event2', 'event3'],
notAfter: ['event4', 'event5'],
},
}

and running our test suite you can grep the output and get something like
this:

2 #AssertionError: Event "close" must fire after at least one of ["error","end","finish","destroy","aborted"], fired []
2 #AssertionError: Event "close" must fire after at least one of ["error","upgrade","aborted","finish","end"], fired ["socket","drain","response"]
2 #AssertionError: Event "close" must fire after at least one of ["listening","error"], fired []
2 #AssertionError: Event "listening" fired 2 times, more than 1
2 #AssertionError: Event "unpipe" must not fire after "error", fired ["pipe","error"]

It's a very simple description, after is better defined: "after any", and
notAfter is "not after any", and there is a mechanism for counting the
times which an event has fired. We probably want the concept of afterAll.

In practice minCount is not used anywhere, and its overhead and potential
to leak (in its current implementation) should discourage use of this
without also growing some concept of a finalization step that could be used
to verify this instead of the current global data structure.

You can also use obj.strictEERegister to extend rule definitions in child
classes.

There probably should also be an obj.strictEEReset mode that resets the
"state machine" for reuse (this already found an issue in net.connect
re-usage).

This is currently based on v0.12 because I found it helpful while
debugging other current bugs. In its current state it's a public API
addition that wouldn't be suitable for v0.12.1, but I do think it's useful
to finish this work on the current stable branch and rename APIs and any
additional events such that we can continue to squash bugs.

Other obvious oversights include missing tests and documentation.

I'm publishing this PR as I'm looking for help from someone in the
community who wants to come and help complete this. As a result, you'll end
up with a lot of more knowledge about how Node.js actually works. I myself
learned quite a bit bootstrapping into this. For everyone else, we'll have
described and along the way documented quite a bit of how Node.js works,

which will make it easier for the next person to come and contribute a fix.

You can view, comment on, or merge this pull request online at:

#13862
Commit Summary

  • events: add StrictEE
  • net: add destroy event
  • streams: resume/pause should not fire after end
  • net: reinitialize duplex on reconnect
  • agent: add warning comment about interface
  • tlswrap: don't emit secure twice
  • http: sockets close transitions via _socketClose

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#13862.
Member

jasnell commented Mar 24, 2015

I recently sat down with Bert and Michael Dawson and had a conversation
around improvements around the internal APIs. That conversation and this
one pair nicely. I'll help where I can. Either later on this evening or
tomorrow I'll dig in a bit deeper in response to your write up here.
On Mar 24, 2015 3:10 PM, "Timothy J Fontaine" notifications@github.com
wrote:

Before we can start stamping out what it means to be "compatible" with
Node.js, we have to start by writing down how we think Node.js works today,
and then verifying that scenario. (cc @dshaw https://github.com/dshaw
@trevnorris https://github.com/trevnorris @jasnell
https://github.com/jasnell @mdawsonibm https://github.com/mdawsonibm
As we've all had conversations about API compatibility in the past)

Since we're event driven the key abstraction/data structure that impacts
all of Node.js is EventEmitter. While we have spent the time to get that
useful and fast, we haven't spent as much time making sure how Node.js
presents its interfaces via the EventEmitter are meeting our expectations.

There are many places we can improve Node.js, but one of our friction
points today (internally and externally) involves around what events will
happen, when, and in what order.

This branch/PR adds the notion of a StrictEE implementation of
EventEmitter that provides facilities to describe your events.

Instead of inheriting from plain old EE you inherit from StrictEE, and
then when constructing you pass in an object which defines the events and
the orders that can be passed in.

I myself am not convinced if this should be used in production, there is
definitely plenty of low hanging fruit for improving its performance (it
should create way fewer object literals). But really, I think (unless the
user has asked for "strict mode") normal operating procedure should be that
most of the strict checks are noops.

Instead, I think this is critical while running our test suite, and
verifying our software (and our downstream users code).

A definition of a rule looks like this currently:

{
myEvent: {
minCount: 1,
maxCount: 2,
after: ['event1', 'event2', 'event3'],
notAfter: ['event4', 'event5'],
},
}

and running our test suite you can grep the output and get something like
this:

2 #AssertionError: Event "close" must fire after at least one of ["error","end","finish","destroy","aborted"], fired []
2 #AssertionError: Event "close" must fire after at least one of ["error","upgrade","aborted","finish","end"], fired ["socket","drain","response"]
2 #AssertionError: Event "close" must fire after at least one of ["listening","error"], fired []
2 #AssertionError: Event "listening" fired 2 times, more than 1
2 #AssertionError: Event "unpipe" must not fire after "error", fired ["pipe","error"]

It's a very simple description, after is better defined: "after any", and
notAfter is "not after any", and there is a mechanism for counting the
times which an event has fired. We probably want the concept of afterAll.

In practice minCount is not used anywhere, and its overhead and potential
to leak (in its current implementation) should discourage use of this
without also growing some concept of a finalization step that could be used
to verify this instead of the current global data structure.

You can also use obj.strictEERegister to extend rule definitions in child
classes.

There probably should also be an obj.strictEEReset mode that resets the
"state machine" for reuse (this already found an issue in net.connect
re-usage).

This is currently based on v0.12 because I found it helpful while
debugging other current bugs. In its current state it's a public API
addition that wouldn't be suitable for v0.12.1, but I do think it's useful
to finish this work on the current stable branch and rename APIs and any
additional events such that we can continue to squash bugs.

Other obvious oversights include missing tests and documentation.

I'm publishing this PR as I'm looking for help from someone in the
community who wants to come and help complete this. As a result, you'll end
up with a lot of more knowledge about how Node.js actually works. I myself
learned quite a bit bootstrapping into this. For everyone else, we'll have
described and along the way documented quite a bit of how Node.js works,

which will make it easier for the next person to come and contribute a fix.

You can view, comment on, or merge this pull request online at:

#13862
Commit Summary

  • events: add StrictEE
  • net: add destroy event
  • streams: resume/pause should not fire after end
  • net: reinitialize duplex on reconnect
  • agent: add warning comment about interface
  • tlswrap: don't emit secure twice
  • http: sockets close transitions via _socketClose

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#13862.
@tjfontaine

This comment has been minimized.

Show comment
Hide comment
@tjfontaine

tjfontaine Mar 24, 2015

Related to this, but not included (and not necessarily in scope of this change) is the idea of separating out what is an internal state machine and what are external events for consumers to subscribe.

While working on getting the PR this far, it's clear that at times we go to great lengths to avoid the scenario where external consumers of add/remove listeners won't adversely effect our internal state machine.

It shouldn't be that during the normal consumption of the EE APIs (i.e. removeAllListeners) results in node not being able to make forward progress.

It also shouldn't be the case that Node.js is worried about adding internal events (especially during patch releases) that might step on external users.

We should be working to clearly delineate between internal and external, and this sort of change gets us closer to even knowing what our surface area actually is.

tjfontaine commented Mar 24, 2015

Related to this, but not included (and not necessarily in scope of this change) is the idea of separating out what is an internal state machine and what are external events for consumers to subscribe.

While working on getting the PR this far, it's clear that at times we go to great lengths to avoid the scenario where external consumers of add/remove listeners won't adversely effect our internal state machine.

It shouldn't be that during the normal consumption of the EE APIs (i.e. removeAllListeners) results in node not being able to make forward progress.

It also shouldn't be the case that Node.js is worried about adding internal events (especially during patch releases) that might step on external users.

We should be working to clearly delineate between internal and external, and this sort of change gets us closer to even knowing what our surface area actually is.
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 27, 2015

Member

@trevnorris ... any reason you can see to keep this open?
Member

jasnell commented Aug 27, 2015

@trevnorris ... any reason you can see to keep this open?
@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Aug 27, 2015

Nope.

trevnorris commented Aug 27, 2015

Nope.

@jasnell jasnell closed this Aug 27, 2015

@orangemocha orangemocha removed the in progress label Aug 27, 2015

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