Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

net: add pauseOnConnect option to createServer() #8576

Closed
wants to merge 1 commit into from
Closed

net: add pauseOnConnect option to createServer() #8576

wants to merge 1 commit into from

Conversation

cjihrig
Copy link

@cjihrig cjihrig commented Oct 18, 2014

Currently, when a server receives a new connection, the underlying socket handle begins reading data immediately. This causes problems when sockets are passed between processes, as data can be read by the first process, and thus never read by the second process. This commit allows sockets that are constructed with a handle to be paused initially.

@trevnorris this is the behavior I referenced on #7905 (comment), which we subsequently discussed on IRC. I plan on looking into UDP sockets next.

Closes #7905. Closes #7784.

UPDATE: After looking into UDP sockets, I don't feel that they suffer from this same problem, as new sockets are not created for each connection.

Currently, when a server receives a new connection, the
underlying socket handle begins reading data immediately.
This causes problems when sockets are passed between processes,
as data can be read by the first process, and thus never read
by the second process. This commit allows sockets that are
constructed with a handle to be paused initially.
@samcday
Copy link

samcday commented Oct 22, 2014

For completeness, could you paste the IRC chat here maybe? I'm wondering what was wrong with with @trevnorris original suggestion here.

I think the idea to hold off starting the underlying handle for a newly connected socket until next-tick after connect has fired, and then only if the socket is still owned by the current process, is a better solution.

With the approach in this PR, you're forced to understand all the semantics behind this whole mess, in order to know that you need to pass pauseOnConnect as an option when creating the net.Server.

@cjihrig
Copy link
Author

cjihrig commented Oct 22, 2014

I don't have the logs, and it looks like the IRC channel's logs stop at the beginning of the month.

My problem with waiting until nextTick() is that it places an artificial constraint on when the socket must be ready to handle data. If the application needs to do anything that takes longer than nextTick(), then the same problem will happen. I understand that this is an extreme edge case, but I think this is a more flexible solution. I think it's also nice to have a way to start a socket in the paused state.

I don't think it's asking too much for a developer to understand the semantics of this case if they intend to develop around it. I think it's simpler to understand "the socket won't start reading until you tell it to" than it is to understand "the socket will start reading immediately, but not until nextTick()."

@trevnorris
Copy link

I'm okay with this. It's more explicit than simply delaying on nextTick().

/cc @tjfontaine @indutny Want your feedback as well.

@cjihrig If no one has responded by next week ping me.

@cjihrig
Copy link
Author

cjihrig commented Oct 27, 2014

@trevnorris here is your next week ping

this._handle.readStop();
this._readableState.flowing = false;
} else
this.read(0);

Choose a reason for hiding this comment

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

style nit: if one part of an if/else uses brackets all of them should. don't worry about fixing it though. i'll take care of it before merging.

trevnorris pushed a commit that referenced this pull request Oct 27, 2014
Currently when a server receives a new connection the underlying socket
handle begins reading data immediately. This causes problems when
sockets are passed between processes, as data can be read by the first
process and thus never read by the second process.

This commit allows sockets that are constructed with a handle to be
paused initially.

PR-URL: #8576
Fixes: #7905
Fixes: #7784
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
@trevnorris
Copy link

Thanks much. Merged in c2b4f48.

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

Successfully merging this pull request may close these issues.

None yet

4 participants