Skip to content
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.

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

ShutdownWrap vs handle.close #32486

Closed
ronag opened this issue Mar 25, 2020 · 15 comments
Closed

ShutdownWrap vs handle.close #32486

ronag opened this issue Mar 25, 2020 · 15 comments
Labels
question Issues that look for answers.

Comments

@ronag
Copy link
Member

ronag commented Mar 25, 2020

I'm trying to understand some of the net code and ran into _final and _destroy.

_final uses a ShutdownWrap while _destroy does a handle.close()

What is the difference?

@ronag ronag added the question Issues that look for answers. label Mar 25, 2020
@sam-github
Copy link
Contributor

Not an expert, but... isn't final and destroy fundamentally different?

Final/shutdown is an ordered operation, that after all data currently queued has transmitted, will do an orderly shutdown of the socket indicating no more outbound data will be sent (but not preventing more data inbound).

destroy just closes the underlying OS handle (an fd on unix). The timing of this wrt. any ongoing data being sent or received is indeterminate, and once the fd is closed it will (of course) cease to exist, so can't be used to send or recv any more data.

What happens on destroy() to all the ongoing send/recv queues is AFAICT not well specified by the streams contracte, and not consistently implemented... thus the stream of issues you are finding related to it. streams are about ordered bi-directional (sometimes) data xfer, and destroy just chops through that.

Given this, that one does an orderly async wrapped operation, and one probably goes much more directly towards uv_close() is what I'd expect, is that what you are seeing?

@ronag
Copy link
Member Author

ronag commented Mar 25, 2020

isn't final and destroy fundamentally different?

Indeed.

socket indicating no more outbound data will be sent (but not preventing more data inbound).

Good. This was not clear to me. Though I'm still a little confused why "shutdown" only affects the outbound side, should I think of it as "shutdown/end outbound"?. Is it possible to shutdown inbound only?

destroy just closes the underlying OS handle (an fd on unix)

Got it.

thus the stream of issues you are finding related to it. streams are about ordered bi-directional (sometimes) data xfer, and destroy just chops through that.

Makes sense.

@sam-github
Copy link
Contributor

https://linux.die.net/man/2/shutdown

^-- can do either or both directions. close() is similar to doing both directions.

"normal" (orderly) usage is read() until EOF on incoming, write until no more data to send, then shutdown(WR) on outgoing. When both EOF received, and shutdown(WR) have ocurred, then close(). Thats at the syscall layer, but all the stack layers above emulate that basic pattern (because that is ultimately what occurs).

abnormal termination is just close(), at syscall layer. .destroy() (maybe... its not well defined) at the node stream layer.

@sam-github
Copy link
Contributor

should I think of it as "shutdown/end outbound"?

yes: .end() maps to shutdown(WR)

@ronag
Copy link
Member Author

ronag commented Mar 25, 2020

Perfect. I think that answered my question. Thanks a lot.

@ronag ronag closed this as completed Mar 25, 2020
@ronag
Copy link
Member Author

ronag commented Mar 25, 2020

@sam-github @addaleax just one more follow up here, _destroy is currently called before _final in the 'end' handler, i.e. it does not wait for shutdown to be called or completed, is this a potential problem?

pseudo example

function onReadableStreamEnd () {
  this.end() // -> process.nextTick -> _final -> shutdown
  this.destroy() // -> _destroy -> close
}

i.e. allowHalfOpen might not be gracefully closing the handle, and calling shutdown after close might be problematic?

it's probably fixed through #31806 but I'm just curious whether this could be a problem in earlier node versions?

@ronag ronag reopened this Mar 25, 2020
@addaleax
Copy link
Member

@ronag Yeah, I would say that’s a potential problem … ideally, the shutdown would complete first and call the _final callback, right? Otherwise this might result in unexpected ECONNRESET errors?

@ronag ronag mentioned this issue Mar 25, 2020
4 tasks
@ronag
Copy link
Member Author

ronag commented Mar 25, 2020

ideally, the shutdown would complete first and call the _final callback,

Yes, the !allowHalfOpen case should probably not directly call destroy and instead let _final invoke it. But doing so causes another bug, see https://github.com/nodejs/node/pull/31806/files#r382938926.

@ronag
Copy link
Member Author

ronag commented Mar 25, 2020

@addaleax also, deferring destroy to _final causes this #32487 in win10.

@sam-github
Copy link
Contributor

I think its a house of cards down there in C++ streams.... TLS is the worse I've seen, because its this complex protocol engine with a top (facing node js) and a bottom (facing another stream... likely a tcp socket, but could be any bi-directional stream), and when a packet comes IN the bottom, it can actually cause stuff to go back OUT even though there is nothing in or out from the js layer yet. And reading from the js layer can cause TLS to wake up and write things out the bottom... it really stresses the "independent bi-directional streams that xform data going in the same direction, but with weak interactions between the in and out directions" paradigm of sockets. It works, seemingly!, but its pretty fragile.

@addaleax
Copy link
Member

Right. I’d be a big fan of re-writing the TLS implementation from scratch… 😄

@jasnell
Copy link
Member

jasnell commented Mar 25, 2020

I'd be a big fan of re-writing the TLS implementation...

Oh most definitely. It's been on my todo list for a while now, especially having done through the whole QUIC implementation.

@sam-github
Copy link
Contributor

I tired to rework the "implicit handshake" to explicit, but never got it quite working, there was always some set of edge test cases that broke when another was fixed, and I ran out of time and energy.

ronag added a commit to nxtedition/node that referenced this issue Mar 25, 2020
When not allowing half open, handle.close would be
invoked before shutdown has been called and
completed causing a potential data race.

Fixes: nodejs#32486 (comment)
@ronag
Copy link
Member Author

ronag commented Mar 26, 2020

Is the handle.close and ShutdownWrap callbacks guaranteed to be asynchronous regardless of platform?

I have some vague memory of this not being the case but wanted to check with someone more knowledgable in this area,

@addaleax
Copy link
Member

@ronag handle.close() is always asynchronous and always succeeds. .shutdown() can succeed synchronously and returns 1 in that case; that’s always the case for HTTP/2. It can also fail synchronously for libuv-backed streams, in that case it returns a negative, non-zero libuv error code.

@ronag ronag closed this as completed in 138eb32 Apr 1, 2020
BethGriggs pushed a commit that referenced this issue Apr 7, 2020
When not allowing half open, handle.close would be
invoked before shutdown has been called and
completed causing a potential data race.

Fixes: #32486 (comment)

PR-URL: #32491
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this issue Apr 12, 2020
When not allowing half open, handle.close would be
invoked before shutdown has been called and
completed causing a potential data race.

Fixes: #32486 (comment)

PR-URL: #32491
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this issue Apr 22, 2020
When not allowing half open, handle.close would be
invoked before shutdown has been called and
completed causing a potential data race.

Fixes: #32486 (comment)

PR-URL: #32491
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Issues that look for answers.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants