fix: make writing large TVP non-blocking operation

[pgraham-acn]

Addresses #475 by adding setImmediate callbacks to allow work waiting on the event loop to process after each parameter and each row of a TVP parameter is written to the internal buffer.

[pgraham-acn]

@arthurschreiber finally got this into a mergeable state. Would really appreciate a review so that if further changes are needed I can get those in ASAP, thanks!

[arthurschreiber]

Hey there! Sorry that it has taken me so long to look into this PR. 🙇

First and foremost, thank you for working on this! ❤️TVPs (and parameters in general) are an area in tedious that has not seen a lot of love, and you've run into one of the "dark corners" with your attempt to write a lot of data via a TVP.

As you figured out, writing parameters is a fully synchronous process, and if the data that is written is too big, it can starve the event loop, leading to serious performance issues for applications that require minimal event loop execution lag.

The solution you're proposing here works by adding callbacks to all parameter methods that write data, and making the calls to makeRequest asynchronous via these callbacks. I can also see that you introduced a new state called BUILDING_CLIENT_REQUEST, into which the connection moves while waiting for the payload to be generated.

I think this solution definitely works and solves the problem you've identified. I'm just not sure if that's the direction I want to take the internals of tedious. I'd much rather see a solution that continues to extend the usage of Streams inside tedious.

E.g. for your use case, a much more "natural" solution would be to allow ReadableStream instances as the values for a TVP parameter. These parameter streams could then be piped into a transform stream that converts the values into the actual TDS protocol data. This way, we'd not only ensure that the event loop does not get starved, we'd also ensure that data is written as early as possible to the Server, and there'd be automatic backoff if the connection to the server becomes overwhelmed. Memory usage of the connection would also be reduced greatly, as we'd never buffer the whole response data, which would also reduce GC pressure.

But all of that is "fantasy talk", while the solution you're proposing here "is a thing" and works. I'm seriously considering to merge this after some more testing, and then work on the "future" streaming solution whenever I get around to it.

@pgraham-acn What do you think?

[pgraham]

Hi Arthur,

Thanks for you detailed response. Now it's my turn to apologize for taking so long to get back to you. Obviously I would love to see this get merged in, as you say this is a real thing that fixes a problem. And, selfishly, having this merged would eliminate complexity from our current developer setups and build process.

Also, we have also had to work around some out of memory errors when writing these large TVP values so am definitely interested in how to make the entire process more memory efficient. Having looked at the code, I'm wondering if a way to make this work would be to change writeable-tracking-buffer into a transform stream that's piped directly into the connection socket.

Again, this doesn't go as far as your ideal since the data would still need to live entirely in memory before being passed into Tedious but it would be a further improvement by eliminating what is essentially a second copy of the data living in the writeable-tracking-buffer.

What would be the problems with this approach?

[RamYadlapalli]

@arthurschreiber Can you merge this?

[pgraham-acn]

@arthurschreiber Bump. What are your thoughts on merging this and moving forward in an incremental way?

[arthurschreiber]

🎉 This PR is included in version 6.2.0 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀