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

Incorrect implementation of .setTimeout - with fix #194

Open
Fabian-Lobnig opened this issue Jun 16, 2024 · 2 comments
Open

Incorrect implementation of .setTimeout - with fix #194

Fabian-Lobnig opened this issue Jun 16, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@Fabian-Lobnig
Copy link

Fabian-Lobnig commented Jun 16, 2024

Description

Your .setTimeout method does not work correctly - when calling .setTimeout the "timeout" event is only called once
Also your .setTimeout only takes write activity into account
The NodeJS implementation however states "timeout" should be called after ms of any inactivity

Steps to reproduce

Steps to reproduce the behavior:

  1. Create TCP Server
  2. Create TCP connection
  3. Call .setTimeout(3000)
  4. Don`t send anything
  5. Received "timeout" after 3000ms as expected
  6. Send something(timeout is not reset)
  7. Wait another 3000ms --> No "timeout" event will be received

Fix

@@ -182,10 +182,11 @@ export default class Socket extends EventEmitter {
      * @param {() => void} [callback]
      */
     setTimeout(timeout, callback) {
-        if (timeout === 0) {
+        this._timeoutMsecs = timeout;
+        if (this._timeoutMsecs === 0) {
             this._clearTimeout();
         } else {
-            this._activateTimer(timeout);
+            this._resetTimeout();
         }
         if (callback) this.once('timeout', callback);
         return this;
@@ -193,15 +194,15 @@ export default class Socket extends EventEmitter {
 
     /**
      * @private
-     * @param {number} [timeout]
      */
-    _activateTimer(timeout) {
-        if (timeout !== undefined) this._timeoutMsecs = timeout;
-        this._clearTimeout();
-        this._timeout = setTimeout(() => {
+    _resetTimeout() {
+        if (this._timeoutMsecs !== 0) {
             this._clearTimeout();
-            this.emit('timeout');
-        }, this._timeoutMsecs);
+            this._timeout = setTimeout(() => {
+                this._clearTimeout();
+                this.emit('timeout');
+            }, this._timeoutMsecs);
+        }
     }
 
     /**
@@ -327,7 +328,6 @@ export default class Socket extends EventEmitter {
      * @return {boolean}
      */
     write(buffer, encoding, cb) {
-        const self = this;
         if (this._pending || this._destroyed) throw new Error('Socket is closed.');
 
         const generatedBuffer = this._generateSendBuffer(buffer, encoding);
@@ -340,7 +340,7 @@ export default class Socket extends EventEmitter {
                 this._msgEvtEmitter.removeListener('written', msgEvtHandler);
                 this._writeBufferSize -= generatedBuffer.byteLength;
                 this._lastRcvMsgId = msgId;
-                if (self._timeout) self._activateTimer();
+                this._resetTimeout();
                 if (this.writableNeedDrain && this._lastSentMsgId === msgId) {
                     this.writableNeedDrain = false;
                     this.emit('drain');
@@ -434,6 +434,7 @@ export default class Socket extends EventEmitter {
      */
     _onDeviceDataEvt = (/** @type {{ id: number; data: string; }} */ evt) => {
         if (evt.id !== this._id) return;
+        this._resetTimeout();
         if (!this._paused) {
             const bufferData = Buffer.from(evt.data, 'base64');
             this._bytesRead += bufferData.byteLength;

Expected

Receive "timeout" event after ms of any inactivity

Relevant information

| OS | any - this is js code |
| react-native-tcp-socket | 6.0.6 |

@Fabian-Lobnig Fabian-Lobnig added the bug Something isn't working label Jun 16, 2024
@Fabian-Lobnig Fabian-Lobnig changed the title Incorrect implementation of .setTimeout Incorrect implementation of .setTimeout - with fix Jun 16, 2024
@Rapsssito
Copy link
Owner

@Fabian-Lobnig, thanks for the feedback! Could you create a PR with your fix?

@Fabian-Lobnig
Copy link
Author

Fabian-Lobnig commented Jun 23, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants