Pausing and resuming is causing the stream to remain stuck

[irgijs]

We are running into an issue where the tedious stream remains stuck if the stream is paused and resumed multiple times. The stream is not resumed even though certain rows should be emitted into the stream pipeline.

It seems to be related to the data as this problem does not occur if the data is changed.

A script is attached to reproduce this problem. An npm install should be performed and the database authentication settings should be adjusted inside the script.

test_pause_and_resume.zip

[mcwonka]

We have the same issue: Stream remains stuck while fetching data.

I looked into it and so far I think there is a timing issue inside incoming-message-stream.js I not about pause and resume in general, calling pause and resume makes it only more likely to happen.

When it is stuck this.currentMessage is undefined before stream ends and when it happens it is almost to the end. Maybe this helps for someone who looks into it.

[irgijs]

We agree on the issue being caused by code somewhere inside incoming-message-stream.js. It seems that this commit triggers or perhaps even causes the error to occur.

[mcwonka]

@irgijs Have you tried version 6.0.1.?

[irgijs]

@mcwonka 6.0.1 does not work for us. Version 6.0.0 and previous versions do work.

[MichaelSun90]

Hi @mcwonka, I was reading through the original thread of the PR #518 for adding pause and resume. That thread mentioned something related pause when there are few entries left: "I have fixed the problem. It happened when there were only a few records left when pause() was called. I have added another test case to verify that this problem is solved. Could you please test again?" I think this part is could related to what @mcwonka mentioned: "when it happens it is almost to the end."

Therefore I through that try to reduce that package size like it is mentioned from the conversion from the original thread. I set an extra value under the connection configuration for reducing the packet Size which indeed resolves the stuck issue from test_pause_and_resume. However, I am not sure whether this is a stable fix or not.

This is the documentation for the configuration key:
options.packetSize
The size of TDS packets (subject to negotiation with the server). Should be a power of 2. (default: 4096).

This is how I modified the configuration:
const config = {
... (add other required connection keys like server, authentication, and so on)
options: {
... (add other required connection keys like encrypt, and so on)
packetSize:2046
}
};

[irgijs]

@MichaelSun90 I applied this workaround to our software but it does not seem to work ([email protected] / SQL Server 2017)

[MichaelSun90]

@irgijs For the test_pause_and_resume.js that you provided, I only add the "packetSize:2046", then the issue seems to be solved. Is it possible that you added the packetsize to the config.authentication.options instead of config.options? It works for me. Without this packet size, the test script always hanging at row 920, after adding this, it seems went all the way.

const config = {
server: "localhost",
authentication: {
type: "default",
options: {
userName: "sa",
password: "sa",
}
},
options:{
packetSize:2046
}
};

[irgijs]

@MichaelSun90 This solution does work for the test_pause_and_resume.js, but not for the software we develop. Therefore I don't think this is a stable fix.

[arthurschreiber]

I'll be taking a look as soon as I can. Changing the packetSize might make this work for some special cases by sheer luck, but it's not really a proper fix. 😅

Thank you for providing a test case, that'll be very helpful in reproducing this!

[arthurschreiber]

Again, thank you for the report and the test case. Based on the test case, I was able to track down the issue and build a proper fix:

diff --git a/src/incoming-message-stream.js b/src/incoming-message-stream.js
index 65c16d3..6c83497 100644
--- a/src/incoming-message-stream.js
+++ b/src/incoming-message-stream.js
@@ -64,13 +64,24 @@ class IncomingMessageStream extends Transform {
         }
 
         if (packet.isLast()) {
-          this.currentMessage = undefined;
-          // Wait until the current message was fully processed before we
-          // continue processing any remaining messages.
-          message.once('end', () => {
-            this.processBufferedData(callback);
-          });
-          message.end(packet.data());
+          const endMessage = () => {
+            this.currentMessage = undefined;
+
+            // Wait until the current message was fully processed before we
+            // continue processing any remaining messages.
+            message.once('end', () => {
+              this.processBufferedData(callback);
+            });
+
+            message.end(packet.data());
+          };
+
+          if (message.isPaused()) {
+            message.on('resume', endMessage);
+          } else {
+            endMessage();
+          }
+
           return;
         } else {
           this.currentMessage = message;

I'll open a proper PR and push a new release as soon as I can. In the meantime, feel free to test using the above patch and let me know if that fixes the issue for you too! 🙇

[irgijs]

@arthurschreiber Thank you for providing a patch. I can confirm your patch does work for our own software as well!

[mcwonka]

@arthurschreiber It also works for us! Thanks!

[arthurschreiber]

@irgijs I just opened #904 with an even simpler fix for this. I'll merge this and release a new version probably tomorrow.

Thanks again for reporting this and for providing all the information to easily reproduce this!

[arthurschreiber]

🎉 This issue has been resolved in version 6.1.2 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[arthurschreiber]

@mcwonka @irgijs I just pushed [email protected] to the next tag, You can install it via npm install tedious@next. It'll automatically be promoted as latest in a few days. Thank you again! 🙇