Subscription Event Engine and Events Listener implementation #196

parfeon · 2024-09-11T09:58:25Z

feat(event-engine): add general event engine

Add core Event Engine implementation with the required set of types and methods.

feat(subscribe-event-engine): add subscribe event engine

Add Subscribe Event Engine built atop of the core Event Engine implementation.

feat(entities): add base PubNub entities

Add the following entities: channel, channel group, uuid and channel metadata objects.

feat(subscription): add subscription and subscription sets

Add objects to manage subscriptions and provides interface for update listeners.

feat(listeners): add event listener

Add new event listeners, which make it possible to add listeners to a specific entity or group of entities (though subscription and subscription set).

feat(retry): add request retry configuration

Added ability to configure automated retry policies for failed requests.

Add core Event Engine implementation with required set of types and methods. feat(subscribe-event-engine): add subscribe event engine Add Subscribe Event Engine built atop of core Event Engine implementation. feat(entities): add base PubNub entities Add following entities: channel, channel group, uuid and channel metadata objects. feat(subscription): add subscription and subscription sets Add objects to manage subscription and provides interface fow updated listeners. feat(listeners): add event listener Add new event listeners which make it possible to add listeners to specific entity or group of entities (though subscription and subscription set).

Xavrax · 2024-09-11T12:40:55Z

10k changes

Xavrax

18/40 files reviewed.

Overall I love your design.
Some questions.

Also it would be nice to have example that shows users how to use the new API.

.github/workflows/run-tests.yml

Xavrax · 2024-09-11T12:43:35Z

core/pbauto_heartbeat.c

@@ -559,7 +560,7 @@ bool pubnub_is_auto_heartbeat_enabled(pubnub_t* pb)
 }


-void pbauto_heartbeat_free_channelInfo(pubnub_t* pb)
+void  pbauto_heartbeat_free_channelInfo(pubnub_t* pb)


whitespace ;o

Xavrax · 2024-09-11T12:44:36Z

core/pbcc_event_engine.c

+//                     Types
+// ----------------------------------------------
+
+// Sharable data type definition.


We use

/** Cryptor algorithm identifier. */

this kind of the inline docs in c-core :/

I tried to find a reason to use this form, but only multiline comments (which is not the case for one-liner) came to my mind. I don't mind. Will change.

Xavrax · 2024-09-11T14:11:44Z

core/pbcc_event_engine.c

+        pbcc_ee_invocation_t* inv = _pbcc_ee_invocation_copy(
+            (pbcc_ee_invocation_t*)pbarray_element_at(invocations, i));
+        if (PBAR_OK != pbarray_add(ee->invocations,inv)) {
+            pbcc_ee_invocation_free(inv);
+            rslt = PNR_OUT_OF_MEMORY;
+        }


As we say that there is no more memory - should we still continue the process?
Shouldn't we somehow only free the allocations and break the loop?

Good catch!
break is missing and failed transition invocation handled few lines below by removing invocations which have been added from this new transition into EE invocations queue.

Xavrax

Seems good (30/40 files review)

Xavrax · 2024-09-12T12:43:47Z

core/pubnub_internal_common.h

@@ -3,6 +3,7 @@
 #if !defined INC_PUBNUB_INTERNAL_COMMON
 #define INC_PUBNUB_INTERNAL_COMMON

+#include <stdbool.h>


I see many random import changes.
Probably they're related to the architecture and feature sets. Am I right?

Some imports have been removed because IDE shown that they are unused, some added in other place.

To be honest, <stdbool.h> and core/c99/stdbool.h: not sure about intention. Do we still need the latest c89 support (as I understand this is the only reason why core/c99/stdbool.h exists because bool is missing from C89 std)?

Moreover, additionally I was confused by feature flags which let specify custom booleans at a compile-time and wasn't sure whether it needed to be utilized in any way.

Do we still need the latest c89 support

This topic is still under the discussion. :/

Xavrax

Overall LGTM

Some new comments.

I'm asking for sub EE sample to progress with the PR ;d.

Xavrax · 2024-09-13T13:35:18Z

lib/pbarray.c

@@ -0,0 +1,643 @@
+/* -*- c-file-style:"stroustrup"; indent-tabs-mode: nil -*- */


Tbh - I would love to have such utilities all across the SDK ;d

Xavrax · 2024-09-13T13:40:14Z

core/pubnub_subscribe_event_engine.c

+        // Temporarily release lock to avoid deadlocks from subscription ee core
+        // code access the list of subscribables.
+        pubnub_mutex_unlock(set->mutw);


I'm a little bit worried about this statement.
Is it possible that this quite short amount of time unguarded mutex might mess anything in execution?

Xavrax · 2024-09-13T13:41:16Z

core/pbcc_subscribe_event_listener.c

+    if (0 == pbref_counter_free(listener->counter)) { free(listener); }
+}


Shouldn't we set listener to NULL?

Xavrax · 2024-09-13T13:45:49Z

core/pbcc_subscribe_event_engine.c

@@ -0,0 +1,2485 @@
+#include "pbcc_subscribe_event_engine.h"


Nitpicking: If I were you, I would split this file into smaller ones. ;d

feat(retry): add retry policy Add failed request automated retry configuration and timer implementations.

Xavrax

There was a lot of files and lines.
I hope that I didn't miss anything.

Overall LGTM.
Some questions and things to do before the merge.

Xavrax · 2024-10-07T12:19:32Z

.clang-format

@@ -88,7 +88,7 @@ PenaltyExcessCharacter: 4
 PenaltyReturnTypeOnItsOwnLine: 100


Do you use format for single files?
I want to reformat the whole repository but there are some issues with compilation so I'm confused how does it work on your side.

Hm, I didn't receive a penalty there.
Also, it already was there and the only thing which I changed was the Standard: value: Cpp11→c++11` (previous value wasn't recognized)

mhm.
Please do not format whole codebase within this PR.

I want to do it at some point, but it makes ~80k lines change :D

CMakeLists.txt

core/pbauto_heartbeat.c

Xavrax · 2024-10-07T12:23:32Z

core/pbcc_memory_utils.c

+void pbcc_free_ptr(void** ptr)
+{
+    if (NULL == ptr || NULL == *ptr) return;
+
+    free(*ptr);
+    *ptr = NULL;
+}


This is quite risky approach.

What in case whereas ptr points to the allocated memory.
I know that propably not in your use-case but I'm aware of the future development and using this function on allocated pointer to allocated pointer (which is ofc very tricky and nobody should do this ofc) might fail.

I can assume some array of pointers that might be failing here.

I will remove this function because the task for which it has been done has been solved in another way :)

Xavrax · 2024-10-07T12:25:33Z

core/pbcc_memory_utils.h

+#define PBCC_ALLOCATE_TYPE(var, type, print_error_message, return_value) \
+    type *var = (type *)malloc(sizeof(type));                            \
+    if (NULL == var) {                                                   \
+        if (true == print_error_message) {                               \
+            PUBNUB_LOG_ERROR("[%s] Failed to allocate memory for '%s'\n",\
+                             __func__, #type);                           \
+        }                                                                \
+        return return_value;                                             \
+    }


I'm aware of macros.
This one seems to be valid but I'm affraid that it might generate very strange compilation errors.

What do you mean by “strange”?

I'm back in the days when compilers couldn't distinguish the generated from macro code from the hand-written ones.
I remember that I had to count lines of macro to find where exactly the problem occurred ;D

But I'm not against macros - just wanted to add a note about it. ;d

core/pubnub_netcore.c

Xavrax · 2024-10-07T12:39:46Z

core/pubnub_entities.c

+{
+    if (NULL == entity || NULL == *entity) { return false; }
+
+    printf("~~~~~~> RESULT: %d\n", is_pubnub_entity_(*entity));


debug print

Xavrax · 2024-10-07T12:41:13Z

core/samples/subscribe_event_engine_sample.c

@@ -0,0 +1,28 @@
+/* -*- c-file-style:"stroustrup"; indent-tabs-mode: nil -*- */


Have you used sanitizers to test it?

If not I can do it for you tomorrow (or any other day).

@Xavrax nope. You asked me to add it, so I just copied everywhere :)
Need to see if there are any for CLion.

core/pbcc_subscribe_event_engine_events.c

core/pubnub_retry_configuration.c

Fixed issue because of which `matched` field had wrong value and length. fix(event-engine): fix consequential subscribe invocations In certain situations, Event Engine contained invocations which held outdated data and shouldn't be called. refactor(demo): completed subscribe event engine and listeners sample application

Xavrax

Great job!
LGTM

Few questions.
Gonna approve right after sanitizers' issues are resolved.

Xavrax · 2024-10-14T15:27:48Z

microchip_harmony/pubnub_ntf_harmony.c

+#if PUBNUB_USE_RETRY_CONFIGURATION
+    if (NULL != pb->core.retry_configuration &&
+        pubnub_retry_configuration_retryable_result_(pb)) {
+        uint16_t delay = pubnub_retry_configuration_delay_(pb);
+
+        if (delay > 0) {
+            if (NULL == pb->core.retry_timer)
+                pb->core.retry_timer = pbcc_request_retry_timer_alloc(pb);
+            if (NULL != pb->core.retry_timer) {
+                pbcc_request_retry_timer_start(pb->core.retry_timer, delay);
+                return;
+            }
+        }
+    }
+
+    /** There were no need to start retry timer, we can free it if exists. */
+    if (NULL != pb->core.retry_timer) {
+        pb->core.http_retry_count = 0;
+        pbcc_request_retry_timer_free(&pb->core.retry_timer);
+    }
+#endif // #if PUBNUB_USE_RETRY_CONFIGURATION


Q: is it possible to merge it in different files to have one universal function?

@Xavrax can you elaborate more on this one? This code is identical in three files - do you want to add new function which have this code inside and use it in those files (function will return boolean to know whether flow should continue or not)?

Xavrax · 2024-10-14T15:28:13Z

openssl/posix.mk

 ifndef USE_CRYPTO_API
-USE_CRYPTO_API = 0
+USE_CRYPTO_API = 1
 endif


Q: Should it be enabled by default? In Cmake default value is OFF?

@Xavrax in current file (at the end of the line) you can see that crypto files were there unconditionally and when guards has been fixed it started to cause problems.
Here it has been enabled by default because CI runs all task which require crypto module to be used - this is the reason why I enabled it in those files.

Xavrax · 2024-10-14T15:31:47Z

core/pbcc_subscribe_v2.c

+        rslt.match_or_group.ptr  = (char*)found.start + 1;
+        rslt.match_or_group.size = found.end - found.start - 2;


Q:
+ " character
- "\n characters

Am I right?

@Xavrax When I tried to find listener for group, I've got something like this: "channel_group\". Then I checked how regular channel parsed, and it looks like there were these fixed added before.

Xavrax · 2024-10-14T16:19:45Z

core/pbcc_request_retry_timer.c

+    pubnub_mutex_lock(timer->mutw);
+    timer->running = true;
+    timer->delay   = delay;
+    pubnub_mutex_unlock(timer->mutw);
+    pubnub_mutex_lock(timer->pb->monitor);
+    timer->pb->state = PBS_WAIT_RETRY;
+    timer->pb->core.http_retry_count++;
+    pubnub_mutex_unlock(timer->pb->monitor);


Q: is it okay to lock those locks separetely?
Won't it cause any UB?

@Xavrax I don't think it is critical, but I can put one of them inside another after sanitizer check (I hope it is not critical).

Fix issue because of which demo app crashed in attempt to free up static strings. refactor(retry-timer): update mutex lock scope Include one of mutex in another (timer inside of PubNub context mutex).

Fix issues because of which library crashes in attempt to move memory into overlapping region.

Xavrax

Works on my side <3

parfeon · 2024-10-15T11:44:28Z

@pubnub-release-bot release

pubnub-release-bot · 2024-10-15T14:01:49Z

🚀 Release successfully completed 🚀

parfeon added status: in progress This issue is being worked on. priority: medium This PR should be reviewed after all high priority PRs. type: feature This PR contains new feature. labels Sep 11, 2024

parfeon self-assigned this Sep 11, 2024

Xavrax reviewed Sep 11, 2024

View reviewed changes

Xavrax reviewed Sep 12, 2024

View reviewed changes

Xavrax reviewed Sep 13, 2024

View reviewed changes

parfeon and others added 2 commits October 2, 2024 21:24

refactor(event-engine): split subscribe event engine to separate files

8ee9f0d

feat(retry): add retry policy Add failed request automated retry configuration and timer implementations.

Merge branch 'master' into CLEN-2083

d6807d6

parfeon marked this pull request as ready for review October 2, 2024 18:34

parfeon requested a review from seba-aln as a code owner October 2, 2024 18:34

parfeon requested a review from Xavrax October 3, 2024 07:30

parfeon added 4 commits October 7, 2024 10:13

refactor(retry-timer): refactor retry timer usage

936c03d

refactor: remove redundant inclusion

6f2ffd4

build: update pre-compile macro

5fb2a09

build: update pre-compile macro

8ccdb12

Xavrax reviewed Oct 7, 2024

View reviewed changes

parfeon added 6 commits October 8, 2024 13:26

wip: fixing threading issues

22490e1

refactor: rever pbpal_mutex.h

934024b

refactor(openssl-make): build with crypto enabled by default

d320625

refactor(cpp-make): add missing PUBNUB_CRYPTO_API guards in interface

7beef7a

refactor(cpp-make): enable PUBNUB_CRYPTO_API by default

7c07bed

Xavrax reviewed Oct 14, 2024

View reviewed changes

parfeon added 4 commits October 14, 2024 20:09

fix(sample): fix subscribe event engine sample

0a1863e

Fix issue because of which demo app crashed in attempt to free up static strings. refactor(retry-timer): update mutex lock scope Include one of mutex in another (timer inside of PubNub context mutex).

fix(sanitizer): fix thread issues reported by the sanitizer

6c7d785

refactor(sample): fix callback for subscription

f7d3fab

fix: fix strln usage on empty block of memory

c94385b

fix(auto-heartbeat): memory issues with member removal

0095a45

Fix issues because of which library crashes in attempt to move memory into overlapping region.

Xavrax approved these changes Oct 15, 2024

View reviewed changes

PubNub SDK v4.14.0 release.

90e6cd0

parfeon merged commit dab23a6 into master Oct 15, 2024
5 checks passed

parfeon deleted the CLEN-2083 branch October 15, 2024 14:01

		@@ -0,0 +1,643 @@
		/* -- c-file-style:"stroustrup"; indent-tabs-mode: nil -- */

		if (0 == pbref_counter_free(listener->counter)) { free(listener); }
		}

		@@ -88,7 +88,7 @@ PenaltyExcessCharacter: 4
		PenaltyReturnTypeOnItsOwnLine: 100

		@@ -0,0 +1,28 @@
		/* -- c-file-style:"stroustrup"; indent-tabs-mode: nil -- */

		rslt.match_or_group.ptr = (char*)found.start + 1;
		rslt.match_or_group.size = found.end - found.start - 2;

Subscription Event Engine and Events Listener implementation #196

Subscription Event Engine and Events Listener implementation #196

Conversation

parfeon commented Sep 11, 2024 • edited Loading

Xavrax commented Sep 11, 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

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

Xavrax Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

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

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

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

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

parfeon Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Xavrax Oct 14, 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

Xavrax left a comment

Choose a reason for hiding this comment

parfeon commented Oct 15, 2024

pubnub-release-bot commented Oct 15, 2024

parfeon commented Sep 11, 2024 •

edited

Loading

Xavrax Sep 13, 2024 •

edited

Loading

parfeon Oct 14, 2024 •

edited

Loading

Xavrax Oct 14, 2024 •

edited

Loading