refactor/EE #210

mohitpubnub · 2024-03-21T09:07:14Z

updated subscribe Effects
Fixed Retry in effect handlers to get configuration from PNConfiguration instance.
Refactored Unsubscribe.
Renamed SubscirbeOperation2 to SubscriptionEndpoint
Miscellaneous minor fixes

* updated subscribe Effects * Fixed Retry in effect handlers to get configuration from PNConfiguration instance. * Refactored Unsubscribe. * Renamed SubscirbeOperation2 to SubscriptionEndpoint * Miscellaneous minor fixes

This reverts commit 7897081.

Xavrax

Some questions and findings.
Overall LGTM - great job!

Xavrax · 2024-03-22T07:36:28Z

src/Api/PubnubApi/EndPoint/PubSub/SubscribeOperation2.cs

@@ -1,869 +0,0 @@
-using System;


I love that you fully removed that cursed file... <3

Xavrax · 2024-03-22T07:37:56Z

src/Api/PubnubApi/EndPoint/PubSub/SubscribeEndpoint.cs

+        private List<string> subscribeChannelNames = new List<string>();
+        private List<string> subscribeChannelGroupNames = new List<string>();


I don't know if is it a good practice or not but I've seen in our code many occurrences of IEnumerable instead of using lists directly. Should we continue this trend?

Here the purpose of using List<string> is, We are going to modify the channels/groups (based on presence channels/groups subscribed or not)
Having List type here makes add/remove or indexer available for further operations.

Not sure about other places, But If we declare IEnumerable here then we will need to convert it to list/array whenever we need to access any element /update it

So It's based on usage we should assign IEnumerable / List/ Array.

Xavrax · 2024-03-22T07:39:45Z

src/Api/PubnubApi/EndPoint/PubSub/SubscribeEndpoint.cs

+            if ((channels == null || channels.Length == 0) && (channelGroups == null || channelGroups.Length == 0))
+            {
+				throw new ArgumentException("Either Channel Or Channel Group or Both should be provided.");
+			}
+
+			if (this.subscribeEventEngineFactory.HasEventEngine(instanceId))
+            {
+                subscribeEventEngine = subscribeEventEngineFactory.GetEventEngine(instanceId);
+			}
+            else


I don't know if it is a github issue but this formatting looks awful :kekw:

I cross checked this.. It looks as expected in IDE

Still Indented & refactored a bit

Xavrax · 2024-03-22T07:41:40Z

src/Api/PubnubApi/Pubnub.cs

+                if (subscribeEventEngineFactory.HasEventEngine(InstanceId))
+                {
+                    var subscribeEventEngine = subscribeEventEngineFactory.GetEventEngine(InstanceId);
+                    subscribeEventEngine.EventQueue.Enqueue(new ReconnectEvent() { Channels = (subscribeEventEngine.CurrentState as SubscriptionState).Channels, ChannelGroups = (subscribeEventEngine.CurrentState as SubscriptionState).ChannelGroups, Cursor = resetSubscribeTimetoken ? null : (subscribeEventEngine.CurrentState as SubscriptionState).Cursor });


This line should be splitted - it's very hard to understand while reading ;/

Xavrax · 2024-03-22T07:43:25Z

src/Api/PubnubApi/EventEngine/Subscribe/Effects/ReceivingEffectHandler.cs

-                Region = response.Item1?.Timetoken.Region,
-                Timetoken = response.Item1?.Timetoken.Timestamp
+                Region = response.Item1?.Timetoken?.Region,
+                Timetoken = response.Item1?.Timetoken?.Timestamp


Maybe null check should be done at the beggining.
I know that it won't faile because of the ? mark but still, there is no need to setup new variables if the response.Item2 is null return the whole function.

Yes, I think null conditional operator here is not necessary and check before creating cursor object makes sense.

jakub-grzesiowski · 2024-03-22T09:08:52Z

src/Api/PubnubApi/EndPoint/PubSub/SubscribeEndpoint.cs

+
+            if (this.subscribeChannelGroupNames == null)
+            {
+                this.subscribeChannelGroupNames = new List<string>();


Nitpick, can be shortened with "??="

jakub-grzesiowski · 2024-03-22T09:29:30Z

src/Api/PubnubApi/EventEngine/Subscribe/SubscribeEventEngineFactory.cs

 			Pubnub pubnubInstance,
 			PNConfiguration pubnubConfiguration,
 			SubscribeManager2 subscribeManager,
 			Action<Pubnub, PNStatus> statusListener = null,
-			Action<Pubnub, PNMessageResult<T>> messageListener = null)
+			Action<Pubnub, PNMessageResult<object>> messageListener= null)


Does the EE implementation still use the "cast all messages to object even though the PNMessageResult is, in theory, generic" approach?

Earlier there used to be problems when user make multiple subscribe calls with different types (may be by mistake sometimes) which can lead to potential cat exception. Using super class/object type, It can be a bit easy for serialisation utility after getting type knowledge from channelType map.

btw I got this approach from one of the old PR which we had missed in past to merge.
And I interpreted it as described

Also It's hard to predict that T in listener callback will match to the type of message we have in results. So it seems keeping it in general type make sense here when we have to serialise it

… issue addressed

mohitpubnub · 2024-03-26T06:42:36Z

Work in progress for acceptance test (in another PR)… to keep things separate.

* Updated AsyncBridge pkg in project file * EventEngine WIP * EventEngine Handshake and Receiving * subscribe listeners * ISubscribeOperation for Subscribe * untangled PubnubCoreBase2 for SubscribeOperation2 * support pubnubpcl and uwp * capture last TT for subscribe when network fails * PNStatus * EventEngine states and transitions WIP * unit tests for EE - unsubscribed and handshake * WIP * Refactored around states and invocations * Replaced EffectInvocationType with EventType * Refactored EffectInvocation and Event * Added invocations * WIP - ContractTesting * ContractTest simpleSubscribe * Replaced Tuple with KeyValuePair for EventEngine tests * HandshakeReconnect handler * Handshake retry attempts * unit tests for HandshakeReconnecting * Added unit tests to Transitions * refactored based on tests * check for timetoken for valid response * Handshakereconnect failure * Updated Transitions * subscribeHandshakeFailure contract test * Removed unused project reference. * Refactored Effect Dispatcher * fix in refactored effect dispatcher * Updated dependency nuget packages * refactor around dispatcher * unwanted namespace * fix HandshakeReconnect IsManaged flag * not required code * removed unused dispatch method * CancelHandshake * CancelReceive and CancelHandshakeReconnect * ReceivingReconnect WIP * ReceivingReconnect * PresenceEventResult * Fix to pass contract test * Eventengine poc refactor (#170) * WIP EE refactor - core classes * WIP - reorganization * WIP - eventQueue * WIP state abstract class to interface * added states and Pascal case property names (#171) * added states and Pascal case property names * HandshakeReconnectingState and other Pascal casing (#172) * HandshakeReconnectingState * EmitStatusInvocation * HandshakeFailedState * Null guard on invocation list * HandshakeStoppedState, ReceiveStoppedState, ReceiveFailedState, ReceivingState and ReceiveReconnectState transitions (#173) * HandshakeStoppedState transitions * ReceiveStoppedState and ReceiveFailedState transitions * ReceivingState transitions * ReceiveReconnectState transitions * Eventengine/state simplification (#174) * WIP remove abstract from interface, add transition utils * Simplified the invocations during transitions, added onEntry and onExit transitions, added HandshakingState and UnsubscribedState * fix * Switch expression * WIP * WIP * WIP * Review fixes * member case * removed With(null) * file ref fix * fix: simplify and add comments * fix: comments * fix: more comments * fix: race condition * fix: additional race condition guards * fix: removed unused property * fix: made loop generic * feat: simplified abstraction (#176) * UnsubscribeAll Event (#175) * Added UnsubscribeAllEvent to states * Eventengine/handshake handler (#181) * feat: simplified abstraction * feat: handshake effect handler * fix: initial retry implementation * handshake reconnect * fix: handshake handler * revert *.csproj changes * added required info in handshake reconnect invocation. * receive handler (#180) * receive handler * fix: background handlers * *reconnection configurations *refactor naming * passing configuration for reconnection * fix: transition values from receive reconnect * wip: receive messages * Revert "Merge remote-tracking branch 'origin/eventengine/handshake-handler' into eventengine/handshake-handler" This reverts commit be568f7, reversing changes made to 0da6c9a. * fix * ReceivingEffectHandler - ReceivingResponse * null check * SubscribeEventEngine - receiveHandler * *csproj file for RReceivingEffectHandler * EmitStatus effect handler - take1 * empty task * wip emit messages * wip emit messages * cleanup and unify convention for *emitters * missing changes emitmessage * Added publisher * emitmessages --------- Co-authored-by: Mohit Tejani <[email protected]> Co-authored-by: Pandu Masabathula <[email protected]> * effect handler cleanup emit status * wip: switch to public * StateTransitions - Tests - WIP * restore overridden changes from 83d3929 * csproj ReconnectionConfiguration to PCL/UWP * wip: base class for states * wip: event engine delegates exposed * refactor handshake unit tests * HandshakeFailedState tests * HandshakeStopped tests * ReceivingState tests * ReceiveReconnectingState tests * ReceiveReconnectingState - more tests * ReceiveFaileState and ReceiveStoppedState * Any UnsubscribeEvent tests * Separated tests to individual states * HandshakingState emit status * invocations and refactor * wip: preliminary potential fix for dispatcher * Refactored HandshakeFailedStateTransition * take-1: integration * listeners * Eventengine/effects abstraction (#184) * wip: effects abstraction overhaul * fix: added comments * fix: emitters * state transition unit tests (#182) * unit test cases for all subscribe state transtiions * moved subscribe event engine initialisation to subscribeOpertaion * added cancel reconnect invocations support * fix: retry condition refined * State transitions for presence event engine (#203) Co-authored-by: Mohit Tejani <[email protected]> Co-authored-by: Mohit Tejani <[email protected]> * temporary suspend EE acceptance tests to unlock progress (#205) * effect invocations used in transitions (#206) Co-authored-by: Mohit Tejani <[email protected]> Co-authored-by: Mohit Tejani <[email protected]> * presence: effects (#208) * start: implementation of presnece event engine effects * added RetryConfiguration class, * removed heartbeat operation from subscribemanager and created a new opeartion class for heartbeat * indentation! * RetryConfiguration definition added * removed old Reconnection settings * review Comment: renamed Heartbeat Operation class name to solve typo * Leave Effect handler * Presence event engine with RetryConfiguration * * Removed RetryConfiguration from Subscription States, Added updated RetryConfiguration settings in Effect handlers * Update PCL project for RetryConfiguration files, leave effect handler * fix: build error system.runtime.remoting not needed * fix: build for UWP, Added missing reference file of latests * revert last UWP csproj changes * Presence EventEngine files link for UWP project * fix: backward compatibility for legacy reconnection policy * fix: reconnectionPolicy class property assignment * indentation and whitespaces review comment * Ee/fix tests (#207) * PresenceEventEngine integration (#209) * PresenceEventEngine and its factory class implementation * PresenceOperation class implementation * Initialise PresenceOperation into SubscribeOperation for triggering PresenceEvent Engine * Subscribe() api to have PresenceOperation initialised conditionally * Added classes for PubnubPCL project * fix: namespace convention for PresenceOperation class * Update UWP.csproj file for new classes reference links * * decoupled wait effect handler from invocation"s interval value * bind effects with States * * `MaintainPresenceState` flag in PubNub configuration. Integration of Unsibscribe api with event engine enabled scenario. Fix state object construction issue for given pubnub instance id. Handling of subsequent Subscribe call in Subscription manager for event engine. removed dummy invocations. Exposed channel/group for subscribe event engine for internal usage. * syntax fix for all.net version compatiblity for empty Array init * refactor/EE (#210) * refactor EventEngine source code: * Updated subscribe states. * updated subscribe Effects * Fixed Retry in effect handlers to get configuration from PNConfiguration instance. * Refactored Unsubscribe. * Renamed SubscirbeOperation2 to SubscriptionEndpoint * Miscellaneous minor fixes * fix csproj file for UWP * Revert "fix csproj file for UWP" This reverts commit 7897081. * fix compilation error csproj UWP * removed duplicate entry for include * fix: subscribe event engine workflow * UnsubscribeAllEndpoint added for event Engine * Refactored code for handling unsubscribe in Presence * added UnsubscribeAllEndpoint class * refactored code for receive Effect handler and some code readablility issue addressed * update PCL and UWP * code indentation pubnub.cs * fix: acceptance test build * CLEN-1787: Event Engine Acceptance Tests (#212) * * fix subscribe event engine transition for restore, * fixed retry related state context information, * fixed timetoken data type for step definition method param * updated code behind file to latest specs. * Presence event engine accpetance tests * fix: Subsscribe event engine related bug caught during acceptance tests * fix: presence event engine issues identified during acceptance tests * fix: objects v2 acceptance tests key issue --------- Co-authored-by: Serhii Mamontov <[email protected]> * fix: key mismatch issue for mock request * encryption test for unencrypted message * hashCode value override for mock Request * PCL: add mock request and comparision implementation for mockserver * PCL:fix value overflow in hash calculation of mock request. * added EventEmitter implementation to fix emit event effect to redirect event data to intended callback code clean up * Fix MockServer Socket and Requests storing issues, link duplicate files from MockServerPubnubApi to MockServerPubnubApiPCL * revert run test change * PubNub SDK v6.20.0.0 release. --------- Co-authored-by: Pandu Masabathula <[email protected]> Co-authored-by: Michał Dobrzański <[email protected]> Co-authored-by: Michał Dobrzański <[email protected]> Co-authored-by: Mateusz Dahlke <[email protected]> Co-authored-by: Serhii Mamontov <[email protected]> Co-authored-by: PUBNUB\jakub.grzesiowski <[email protected]> Co-authored-by: PubNub Release Bot <[email protected]>

mohitpubnub added 2 commits March 21, 2024 14:32

refactor EventEngine source code: * Updated subscribe states.

4ec339f

* updated subscribe Effects * Fixed Retry in effect handlers to get configuration from PNConfiguration instance. * Refactored Unsubscribe. * Renamed SubscirbeOperation2 to SubscriptionEndpoint * Miscellaneous minor fixes

fix csproj file for UWP

7897081

mohitpubnub self-assigned this Mar 21, 2024

mohitpubnub added the status: in progress This issue is being worked on. label Mar 21, 2024

mohitpubnub added 3 commits March 21, 2024 14:55

Revert "fix csproj file for UWP"

8c78c11

This reverts commit 7897081.

fix compilation error csproj UWP

dd10ff0

removed duplicate entry for include

03ab8dc

Xavrax approved these changes Mar 22, 2024

View reviewed changes

fix: subscribe event engine workflow

a1ac119

jakub-grzesiowski reviewed Mar 22, 2024

View reviewed changes

mohitpubnub added 6 commits March 26, 2024 12:01

UnsubscribeAllEndpoint added for event Engine

5cc5574

Refactored code for handling unsubscribe in Presence

687a98c

added UnsubscribeAllEndpoint class

00012e8

refactored code for receive Effect handler and some code readablility…

aef3db6

… issue addressed

update PCL and UWP

d720695

code indentation pubnub.cs

24ecc9b

mohitpubnub added status: done This issue is considered resolved. and removed status: in progress This issue is being worked on. labels Mar 26, 2024

fix: acceptance test build

9f7f268

mohitpubnub merged commit 8789efb into ee/integration Mar 27, 2024
3 of 4 checks passed

mohitpubnub deleted the CLEN-1918 branch March 27, 2024 05:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor/EE #210

refactor/EE #210

mohitpubnub commented Mar 21, 2024

Xavrax left a comment

Xavrax Mar 22, 2024

Xavrax Mar 22, 2024

mohitpubnub Mar 26, 2024

mohitpubnub Mar 26, 2024

Xavrax Mar 22, 2024

mohitpubnub Mar 26, 2024

mohitpubnub Mar 26, 2024 •

edited

Loading

mohitpubnub Mar 26, 2024

Xavrax Mar 22, 2024

mohitpubnub Mar 26, 2024

Xavrax Mar 22, 2024

mohitpubnub Mar 26, 2024

mohitpubnub Mar 26, 2024

jakub-grzesiowski Mar 22, 2024

mohitpubnub Mar 26, 2024

jakub-grzesiowski Mar 22, 2024

mohitpubnub Mar 22, 2024 •

edited

Loading

mohitpubnub Mar 22, 2024

mohitpubnub commented Mar 26, 2024 •

edited

Loading

		private List<string> subscribeChannelNames = new List<string>();
		private List<string> subscribeChannelGroupNames = new List<string>();

refactor/EE #210

refactor/EE #210

Conversation

mohitpubnub commented Mar 21, 2024

Xavrax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohitpubnub Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohitpubnub Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohitpubnub commented Mar 26, 2024 • edited Loading

mohitpubnub Mar 26, 2024 •

edited

Loading

mohitpubnub Mar 22, 2024 •

edited

Loading

mohitpubnub commented Mar 26, 2024 •

edited

Loading