Skip to content

Cleanup stream state in net #465

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
chrisdickinson wants to merge 6 commits into from
Closed

Cleanup stream state in net #465

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f5c5ebe
doc: add release GPG fingerprint for chrisdickinson
chrisdickinson Feb 3, 2015
1d8ad7d
net: do not reach through writableState for decodeStrings
chrisdickinson Jan 16, 2015
0e02aaf
net: use pause() instead of changing _readableState.flowing
chrisdickinson Jan 16, 2015
cc402e5
net: do not reach through readableState for ended, do not maniplate .…
chrisdickinson Jan 16, 2015
30cce39
net,stream,tls: nix writableState.errorEmitted
chrisdickinson Jan 16, 2015
f05d1eb
style fixes
chrisdickinson Feb 4, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
net,stream,tls: nix writableState.errorEmitted
  • Loading branch information
@chrisdickinson
chrisdickinson committed Feb 4, 2015
commit 30cce396c43ef5ef37ad9f26a1d80e8a01b29bb0
4 changes: 0 additions & 4 deletions lib/_stream_writable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,6 @@ function WritableState(options, stream) {
// emit prefinish if the only thing we're waiting for is _write cbs
// This is relevant for synchronous Transform streams
this.prefinished = false;

// True if the error was already emitted and should not be thrown again
this.errorEmitted = false;
}

WritableState.prototype.getBuffer = function writableStateGetBuffer() {
Expand Down Expand Up @@ -299,7 +296,6 @@ function onwriteError(stream, state, sync, er, cb) {
cb(er);
}

stream._writableState.errorEmitted = true;
stream.emit('error', er);
}

Expand Down
4 changes: 2 additions & 2 deletions lib/_tls_wrap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,9 +306,9 @@ TLSSocket.prototype._init = function(socket) {
}

this.ssl.onerror = function(err) {
if (self._writableState.errorEmitted)
if (self._errorEmitted)
return;
self._writableState.errorEmitted = true;
self._errorEmitted = true;
Copy link
Copy Markdown
Member

@indutny indutny Feb 3, 2015

Choose a reason for hiding this comment

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

Hm... This was a bug fix, as far as I remember. Why doesn't it apply anymore?


// Destroy socket if error happened before handshake's finish
if (!this._secureEstablished) {
Expand Down
9 changes: 6 additions & 3 deletions lib/net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ function initSocketHandle(self) {
function Socket(options) {
if (!(this instanceof Socket)) return new Socket(options);

this._errorEmitted = false;
Copy link
Copy Markdown
Member

@indutny indutny Feb 3, 2015

Choose a reason for hiding this comment

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

I see, you moved it here.

this._connecting = false;
this._hadError = false;
this._handle = null;
Expand Down Expand Up @@ -420,12 +421,14 @@ Socket.prototype._destroy = function(exception, cb) {
var self = this;

function fireErrorCallbacks() {
var hadSeenError = self._errorEmitted;
self._errorEmitted = self._errorEmitted || Boolean(exception);
Copy link
Copy Markdown
Member

@indutny indutny Feb 3, 2015

Choose a reason for hiding this comment

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

Please replace Boolean(exception) with !!exception, this is what we do everywhere.

Copy link
Copy Markdown
Member

@indutny indutny Feb 3, 2015

Choose a reason for hiding this comment

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

What is the reason of introducing it here?

Copy link
Copy Markdown
Contributor Author

@chrisdickinson chrisdickinson Feb 4, 2015

Choose a reason for hiding this comment

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

This moves the _errorEmitted flag functionality down to the net.Socket class, since only it and tls.TLSSocket are making use of it, and are able to set it themselves apart from the parent class. This is part of a larger effort to reduce the number of places that stream subclasses reach into parent private state.

Copy link
Copy Markdown
Contributor Author

@chrisdickinson chrisdickinson Feb 4, 2015

Choose a reason for hiding this comment

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

Ah, if you mean hadSeenError, we want to set that before any callbacks fire, to ensure that the outside world sees the stream in the proper state. However, we still want to inspect the original value and switch our behavior based on that.

if (cb) cb(exception);
if (exception && !self._writableState.errorEmitted) {
else if (exception && !hadSeenError) {
Copy link
Copy Markdown
Member

@indutny indutny Feb 3, 2015

Choose a reason for hiding this comment

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

This code style does look a bit different from what we usually do. Why not:

if (cb) {
  cb(exception);
} else if (...) {
}

?

Copy link
Copy Markdown
Contributor Author

@chrisdickinson chrisdickinson Feb 4, 2015

Choose a reason for hiding this comment

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

Ah, since it was a single ExpressionStatement I preserved the braceless if. Fixing.

process.nextTick(function() {
self.emit('error', exception);
});
self._writableState.errorEmitted = true;
self._errorEmitted = true;
}
};

Expand Down Expand Up @@ -840,7 +843,7 @@ Socket.prototype.connect = function(options, cb) {
this._writableState.ended = false;
this._writableState.ending = false;
this._writableState.finished = false;
this._writableState.errorEmitted = false;
this._errorEmitted = false;
this.destroyed = false;
this._handle = null;
}
Expand Down