Merge streams handling code for http2 streams & net.Socket

[addaleax]

Hey :)

It would be great if we could unify all the code from net and http2 that is only concerned with pushing data to/from the underlying stream, ideally into a common base class of net.Socket and Http2Stream, so that we could also maybe port some of the other native streams (zlib, fs) to using StreamBase on the native side & generally just share a lot of code.

This is probably not an easy task, and probably not doable in one pass, because the http2 and net implementations are always just slightly different, but if anybody wants to try to start working on this, feel free to reach out by commenting here.

[inidaname]

Hello
@addaleax I will like to take this up, if you point me to the right files
Thank You

[apapirovski]

@addaleax Are you sure this is a good first issue? This relies on knowledge of both JS & C++, and pretty good grasp of Node streams, net & http2. I would keep it as help wanted but can't imagine someone would want to start contributing with this particular issue.

[addaleax]

@apapirovski Yeah, I would talk with people who volunteer for it in more detail about that :) Then again, this doesn’t really require C++ knowledge, although that’s certainly helpful – it’s only about refactoring the JS, as the C++ parts should be identical between those two systems…

I’ve removed the label for now.

@inidaname The relevant code is in lib/net.js and lib/internal/http2/core.js. As a start, you might want to just try to make _write look identical for both of these, so that they can be merged into being the same function.

I think we’d want to create a new file internal/ for a resulting new base class, but that doesn’t have to happen right now.

[inidaname]

@addaleax Okay thank you

[SirR4T]

Hi @addaleax : would like to take a shot at this, though would need some beginner help too.

For instance, _write in lib/internal/http2/core.js seems to use LibuvStreamWrap, whereas in lib/internal/net.js, it seems to use fs.write(). Is the intent of this issue to change lib/internal/net.js 's _write to use LibuvStreamWrap instead?

[addaleax]

would like to take a shot at this

@SirR4T Sure! But maybe take a different bit of the code, unless @inidaname says that they aren’t working on it anymore.

For instance, _write in lib/internal/http2/core.js seems to use LibuvStreamWrap

Well, the underlying stream isn’t a LibuvStreamWrap, it’s an Http2Stream – that shouldn’t matter in practice, because both of these C++ classes use a common interface, which is exposed via the StreamBase class. (Side note: You probably don’t have to modify C++ for this in any way.)

whereas in lib/internal/net.js, it seems to use fs.write().

The code from lib/internal/net.js isn’t what I’m thinking about – it can probably stay as it is.

For net.Socket, the main implementation of _write() and _writev() can be found in lib/net.js and looks a lot more similar to what’s used in lib/internal/http2/core.js.

Is the intent of this issue to change lib/internal/net.js 's _write to use LibuvStreamWrap instead?

There’s quite a few common pieces between lib/net.js and lib/internal/http2.js that just deal with how the StreamBase C++ API is matched to the “normal” Node streams API.

_write() and _writev() are part of that, but there is other code (e.g. onread in lib/net.js and onStreamRead in lib/internal/http2/core.js) that could also be merged. Maybe take a look at that first? It’s going to be trickier than the write functions, but I hope it’s independent enough for that to work.

If that sounds like a lot, or turns out to be too tricky: A first part of this might be getting rid of the _socketEnd event in lib/net.js. It should be possible to figure out in onread whether self.destroy() should be called or not, based on the writable state of the stream, so we don’t need to create an event listener in afterShutdown. I think this is essentially being done in #19241

[inidaname]

Hello @addaleax
Thank you for the chance but I guess @SirR4T understand the direction been battling with other projects for sometime will be okay if you can take it up
Go for it.

[addaleax]

@SirR4T Are you still interested in this?

[SirR4T]

I am, but looks like I may be out of my depth here. Maybe further hints might help? Or I'll try my hand at other issues first.

[addaleax]

@SirR4T I think you could break this down into small pieces, if that helps. And please don’t be shy to approach me with questions if you have any.

For example, once discrepancy here is that net.js uses .destroy() if there was a write error:

node/lib/net.js

Lines 778 to 779 in 22b6804

	if (err)
	return this.destroy(errnoException(err, 'write', req.error), cb);

Whereas HTTP/2 uses a plain throw:

node/lib/internal/http2/core.js

Lines 1679 to 1680 in 22b6804

	if (err)
	throw errnoException(err, 'write', req.error);

We should probably change http2 to use .destroy() too, because we don’t handle exceptions coming from inside _write() callbacks very well. (There’s another similar instance in the HTTP/2 code.)

Also, it looks like those lines in the HTTP/2 implementation aren’t covered by our tests: https://coverage.nodejs.org/coverage-0048169f5e6d280f/root/internal/http2/core.js.html

Adding a test for them would be a nice change to go along with aligning the implementations, if you feel up for it.

[SirR4T]

Hey @addaleax , I think i got the .destroy() part correct, but I'm still baffled on how to add test cases.

I would think I should induce a situation where createWriteReq() fails, and then assert that indeed stream is destroyed. I attempted to track when would createWriteReq() fail, but I don't see where the C++ side either returns any errno, or throws any exception.

[addaleax]

but I'm still baffled on how to add test cases.

Hm – I guess leave it out then first. I thought you could make this fail by breaking the underlying connection before the write, but a) I missed that HTTP/2 streams are still always asynchronous and b) a broken connection would probably destroy the HTTP/2 stream anyway.

[SirR4T]

Hey @addaleax ! what should be the next steps here?

[SirR4T]

Hi @addaleax , I was just taking a second look at the onread() and onStreamRead() (in lib/net.js and lib/http2/core) functions. It seems to me that if lib/net.js started adopting more symbols, like [kOwner], [kMaybeDestroy], then maybe a single function could be reused on both net.Socket and Http2Stream objects. Is that the correct direction forward?

[jasnell]

Ping @addaleax ... any progress here?

[apapirovski]

Seems like we had some decent progress here. Not sure there's a ton more we can easily do. I'm going to close out for now but feel free to reopen if you believe I've made a mistake.

[addaleax]

Not sure there's a ton more we can easily do.

Yeah, I don’t think we’re even halfway where I want to be. 😄 I’ll work more on this bit in the near future.

[jamesgeorge007]

@addaleax I would like to give it a shot as well 👍
What portion would you like me to take up?

[DamianRivas]

@addaleax is help needed in closing this issue?

[Ethan-Arrowood]

If there are any outstanding parts you want merged together I'd like to try it out too!

[jagtesh]

I would love to help out as well; I'm comfortable working on both C++ and JS parts.

[BridgeAR]

@ronag I guess you might be interested in this.

[sachin2605]

I am interested as well..!! Please let me know if i can

…

On Sat, 4 Jan, 2020, 08:20 Ruben Bridgewater, ***@***.***> wrote: @ronag <https://github.com/ronag> I guess you might be interested in this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19060?email_source=notifications&email_token=AAUACRYDUBWBLEYRTLAJ663Q372OPA5CNFSM4ES3BJN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEICPLWY#issuecomment-570750427>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAUACR3X4Q3RK7NJTMLJXFTQ372OPANCNFSM4ES3BJNQ> .