Fix record discard/delete & subscribe race #209

ronag · 2016-08-09T20:35:53Z

This is my try at fixing the discard/delete and subscribe data-races. #204

The basic idea is that the record can revive itself AFTER successful discard/delete, using the pending queue for the time between re-subscription and revival.

This ensures that there CAN BE ONLY ONE active record instance for the client avoiding the:

UNSOLICITED_MESSAGE
UNSOLICITED_MESSAGE
ACK_TIMEOUT
RESPONSE_TIMEOUT
MULTIPLE_SUBSCRIBTIONS

error cycle

ronag · 2016-08-09T20:39:22Z

src/record/record-handler.js

-
-			return;
-		}
-


Parts of this is probably still needed. In particular the MESSAGE_DENIED part.

'between another clients creation and read message for the same record' is unclear.

coveralls · 2016-08-09T20:43:19Z

Coverage decreased (-0.003%) to 97.243% when pulling ed77f03 on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

ronag · 2016-08-09T20:46:02Z

travis fails since the test doesn't allow re-using/reviving record instances.

coveralls · 2016-08-09T20:57:41Z

Coverage decreased (-0.003%) to 97.243% when pulling e7c4f4c on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

coveralls · 2016-08-09T21:06:53Z

Coverage decreased (-0.02%) to 97.226% when pulling e8eaec8 on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

coveralls · 2016-08-09T21:07:31Z

Coverage decreased (-0.02%) to 97.226% when pulling e8eaec8 on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

coveralls · 2016-08-09T21:08:10Z

Coverage decreased (-0.02%) to 97.226% when pulling 1389963 on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

coveralls · 2016-08-09T21:08:32Z

Coverage decreased (-0.02%) to 97.226% when pulling 1389963 on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

coveralls · 2016-08-09T21:09:47Z

Coverage decreased (-0.02%) to 97.226% when pulling 1389963 on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

coveralls · 2016-08-09T21:15:02Z

Coverage decreased (-0.02%) to 97.226% when pulling c2fab75 on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

coveralls · 2016-08-09T21:20:17Z

Coverage decreased (-65.8%) to 31.458% when pulling 9209a60 on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

coveralls · 2016-08-10T09:26:41Z

Coverage decreased (-0.02%) to 97.227% when pulling 2aaad33 on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

coveralls · 2016-08-10T09:31:02Z

Coverage decreased (-0.02%) to 97.227% when pulling c249ebc on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

ronag · 2016-08-10T12:57:22Z

This also resolve the issue of spamming subscribe/unsubscribe as there is a natural throttling.

coveralls · 2016-08-10T13:22:24Z

Coverage decreased (-0.02%) to 97.224% when pulling 58dba54 on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

coveralls · 2016-08-10T13:24:15Z

Coverage decreased (-0.02%) to 97.224% when pulling 58dba54 on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

coveralls · 2016-08-11T08:58:43Z

Coverage decreased (-0.003%) to 97.243% when pulling 90c98b0 on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

coveralls · 2016-08-11T09:13:05Z

Coverage decreased (-0.005%) to 97.242% when pulling 6b1a649 on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

…subscribe behaviour to separate PR.

coveralls · 2016-08-11T09:13:32Z

Coverage decreased (-0.005%) to 97.242% when pulling 6b1a649 on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

ronag · 2016-08-12T09:20:06Z

src/record/record.js

+		this._sendRead();
+	}
+	else {
+		this._clearTimeouts();


Clear timeout beredda to be moved to top and always be invoked.

coveralls · 2016-08-16T11:54:18Z

Coverage decreased (-0.005%) to 97.242% when pulling 8641f5a on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

coveralls · 2016-08-19T08:58:59Z

Coverage decreased (-0.003%) to 97.243% when pulling 462440b on nxtedition:fix/#204 into f8341be on deepstreamIO:master.

ronag · 2016-08-19T09:01:29Z

@yasserf: tests pass

ronag · 2016-08-31T15:11:17Z

@yasserf: This PR has a race when disconnecting the client before the UNSUBSCRIBEDack. I'm currently in a state where I have no working solution or workaround to the issue #214.

ronag · 2016-09-01T22:35:05Z

I'm closing this as it's broken.

ronag · 2016-09-03T21:44:03Z

Fixed issue.

ronag · 2016-09-03T21:44:51Z

src/utils/resubscribe-notifier.js

 	if( state === C.CONNECTION_STATE.OPEN && this._isReconnecting === true ) {
 		this._isReconnecting = false;
 		this._resubscribe();
 	}
+	else if ( this._isReconnecting === false ) {


We always want to reconnect when going from non OPEN to OPEN.

ronag · 2016-09-03T21:57:20Z

#232

ronag · 2016-09-04T15:05:27Z

src/record/record.js

-	this._readAckTimeout = setTimeout( this._onTimeout.bind( this, C.EVENT.ACK_TIMEOUT ), this._options.recordReadAckTimeout );
-	this._readTimeout = setTimeout( this._onTimeout.bind( this, C.EVENT.RESPONSE_TIMEOUT ), this._options.recordReadTimeout );
-	this._sendRead();
+	this._resubscribeNotifier = new ResubscribeNotifier( this._client, this._sendRead.bind( this ), this._clearTimeouts.bind( this ), true );


Simply doing a clearTimeouts on disconnect is not good enough...

ronag · 2016-09-04T15:06:21Z

I don't think this PR is good enough. But the basic ideas are there. There are some edge cases that I don't think are properly handled.

yasserf · 2016-09-04T15:20:13Z

Yeah, we need to look into the discard/delete async case in more detail

ronag · 2017-02-17T15:03:07Z

This needs to be solved in another way. We've resolved it locally in our fork in a different way.

yasserf added the awaiting review label Aug 9, 2016

ronag reviewed Aug 9, 2016
View reviewed changes

ronag mentioned this pull request Aug 10, 2016

discard/delete data race when quickly re-subscribing #204

Closed

ronag force-pushed the fix/#204 branch from 2aaad33 to c249ebc Compare August 10, 2016 09:29

Fixes deepstreamIO#204. discard/delete & subscribe race.

c249ebc

use whenReady for discard/delete

3c55e72

Cosmetics

58dba54

Cosmetics

90c98b0

Reviving is essentially the same as re-subscribing. Leave chaning re-…

5c2a719

…subscribe behaviour to separate PR.

ronag reviewed Aug 12, 2016
View reviewed changes

Always clear timeouts on _destroy

8641f5a

ronag mentioned this pull request Aug 19, 2016

requestIdleCallback #200

Closed

Tests pass.

462440b

ronag changed the title ~~Fixed record discard/delete handling.~~ Fix record discard/delete & subscribe race Aug 19, 2016

ronag mentioned this pull request Aug 19, 2016

record - re-subscription can be sent READ after UNSUBSCRIBE or DELETE #211

Closed

This was referenced Aug 28, 2016

Asynchronous Races #228

Closed

set isDestroyed = true and isReady flag before delete event #77

Closed

ronag closed this Sep 1, 2016

yasserf removed the awaiting review label Sep 1, 2016

ronag reopened this Sep 3, 2016

yasserf added the awaiting review label Sep 3, 2016

ronag reviewed Sep 3, 2016
View reviewed changes

ronag force-pushed the fix/#204 branch from fdf1afb to 5a8351f Compare September 3, 2016 21:51

ronag mentioned this pull request Sep 3, 2016

disconnect should clear timeouts #232

Closed

Fixed record reconnecting.

08c274b

ronag force-pushed the fix/#204 branch from 5a8351f to 08c274b Compare September 3, 2016 21:57

ronag reviewed Sep 4, 2016
View reviewed changes

ronag closed this Feb 17, 2017

ronag deleted the fix/#204 branch November 14, 2018 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix record discard/delete & subscribe race #209

Fix record discard/delete & subscribe race #209

ronag commented Aug 9, 2016 •

edited

Loading

ronag Aug 9, 2016 •

edited

Loading

coveralls commented Aug 9, 2016

ronag commented Aug 9, 2016 •

edited

Loading

coveralls commented Aug 9, 2016

coveralls commented Aug 9, 2016

coveralls commented Aug 9, 2016

coveralls commented Aug 9, 2016

coveralls commented Aug 9, 2016

coveralls commented Aug 9, 2016

coveralls commented Aug 9, 2016

coveralls commented Aug 9, 2016

coveralls commented Aug 10, 2016

coveralls commented Aug 10, 2016

ronag commented Aug 10, 2016 •

edited

Loading

coveralls commented Aug 10, 2016

coveralls commented Aug 10, 2016

coveralls commented Aug 11, 2016

coveralls commented Aug 11, 2016

coveralls commented Aug 11, 2016

ronag Aug 12, 2016

coveralls commented Aug 16, 2016

coveralls commented Aug 19, 2016

ronag commented Aug 19, 2016

ronag commented Aug 31, 2016 •

edited

Loading

ronag commented Sep 1, 2016

ronag commented Sep 3, 2016

ronag Sep 3, 2016

ronag commented Sep 3, 2016

ronag Sep 4, 2016

ronag commented Sep 4, 2016

yasserf commented Sep 4, 2016

ronag commented Feb 17, 2017

Fix record discard/delete & subscribe race #209

Fix record discard/delete & subscribe race #209

Conversation

ronag commented Aug 9, 2016 • edited Loading

ronag Aug 9, 2016 • edited Loading

Choose a reason for hiding this comment

coveralls commented Aug 9, 2016

ronag commented Aug 9, 2016 • edited Loading

coveralls commented Aug 9, 2016

coveralls commented Aug 9, 2016

coveralls commented Aug 9, 2016

coveralls commented Aug 9, 2016

coveralls commented Aug 9, 2016

coveralls commented Aug 9, 2016

coveralls commented Aug 9, 2016

coveralls commented Aug 9, 2016

coveralls commented Aug 10, 2016

coveralls commented Aug 10, 2016

ronag commented Aug 10, 2016 • edited Loading

coveralls commented Aug 10, 2016

coveralls commented Aug 10, 2016

coveralls commented Aug 11, 2016

coveralls commented Aug 11, 2016

coveralls commented Aug 11, 2016

ronag Aug 12, 2016

Choose a reason for hiding this comment

coveralls commented Aug 16, 2016

coveralls commented Aug 19, 2016

ronag commented Aug 19, 2016

ronag commented Aug 31, 2016 • edited Loading

ronag commented Sep 1, 2016

ronag commented Sep 3, 2016

ronag Sep 3, 2016

Choose a reason for hiding this comment

ronag commented Sep 3, 2016

ronag Sep 4, 2016

Choose a reason for hiding this comment

ronag commented Sep 4, 2016

yasserf commented Sep 4, 2016

ronag commented Feb 17, 2017

ronag commented Aug 9, 2016 •

edited

Loading

ronag Aug 9, 2016 •

edited

Loading

ronag commented Aug 9, 2016 •

edited

Loading

ronag commented Aug 10, 2016 •

edited

Loading

ronag commented Aug 31, 2016 •

edited

Loading