Skip to content

Make pstm handling thread sleep time configurable #267

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.

Sign up for GitHub

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

Merged
pnoltes merged 1 commit into from
Jul 13, 2020
Merged

Make pstm handling thread sleep time configurable #267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
#include "pubsub_admin.h"
#include "../../pubsub_admin_udp_mc/src/pubsub_udpmc_topic_sender.h"

#define PSTM_PSA_HANDLING_SLEEPTIME_IN_SECONDS 30L
#define PSTM_PSA_HANDLING_DEFAULT_SLEEPTIME_IN_SECONDS 30L

#ifndef UUID_STR_LEN
#define UUID_STR_LEN 37
Expand Down Expand Up @@ -79,6 +79,7 @@ celix_status_t pubsub_topologyManager_create(celix_bundle_context_t *context, ce

manager->loghelper = logHelper;
manager->verbose = celix_bundleContext_getPropertyAsBool(context, PUBSUB_TOPOLOGY_MANAGER_VERBOSE_KEY, PUBSUB_TOPOLOGY_MANAGER_DEFAULT_VERBOSE);
manager->handlingThreadSleepTime = celix_bundleContext_getPropertyAsLong(context, PUBSUB_TOPOLOGY_MANAGER_HANDLING_THREAD_SLEEPTIME_SECONDS_KEY, PSTM_PSA_HANDLING_DEFAULT_SLEEPTIME_IN_SECONDS);

@abroekhuis abroekhuis Jul 9, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering, doing this in the create makes it impossible to update the timeout when the component is active.
Getting it where needed, makes this possible. (I doubt this use case is needed..)
Any specific reasons to do it here and not in the handler thread?

@Oipo Oipo Jul 9, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No specific reason, no. Just another case of ctrl+C, ctrl+V.

@Oipo Oipo Jul 9, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the overhead of calling celix_bundleContext_getPropertyAsLong() matters much, but that's the only reason I can think of not wanting to do it inside of the loop.

@Oipo Oipo Jul 9, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe thread-safety? Hrm.

@abroekhuis abroekhuis Jul 9, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this solution, if it ever comes up again, we can always take a look at potential risks. I doubt anyone is going to change the timeout at runtime.


manager->psaHandling.running = true;
celixThread_create(&manager->psaHandling.thread, NULL, pstm_psaHandlingThread, manager);
Expand Down Expand Up @@ -1107,7 +1108,7 @@ static void *pstm_psaHandlingThread(void *data) {
pstm_findPsaForEndpoints(manager); //trying to find psa and possible set for endpoints with no psa

celixThreadMutex_lock(&manager->psaHandling.mutex);
celixThreadCondition_timedwaitRelative(&manager->psaHandling.cond, &manager->psaHandling.mutex, PSTM_PSA_HANDLING_SLEEPTIME_IN_SECONDS, 0L);
celixThreadCondition_timedwaitRelative(&manager->psaHandling.cond, &manager->psaHandling.mutex, manager->handlingThreadSleepTime, 0L);
running = manager->psaHandling.running;
celixThreadMutex_unlock(&manager->psaHandling.mutex);
}
Expand Down
2 changes: 2 additions & 0 deletions bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "pubsub/subscriber.h"

#define PUBSUB_TOPOLOGY_MANAGER_VERBOSE_KEY "PUBSUB_TOPOLOGY_MANAGER_VERBOSE"
#define PUBSUB_TOPOLOGY_MANAGER_HANDLING_THREAD_SLEEPTIME_SECONDS_KEY "PUBSUB_TOPOLOGY_MANAGER_HANDLING_THREAD_SLEEPTIME_SECONDS"
#define PUBSUB_TOPOLOGY_MANAGER_DEFAULT_VERBOSE false


Expand Down Expand Up @@ -76,6 +77,7 @@ typedef struct pubsub_topology_manager {

celix_log_helper_t *loghelper;

unsigned handlingThreadSleepTime;
bool verbose;
} pubsub_topology_manager_t;

Expand Down
6 changes: 0 additions & 6 deletions bundles/pubsub/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,6 @@ if (BUILD_PUBSUB_PSA_UDP_MC)
add_test(NAME pstm_deadlock_udpmc_test COMMAND pstm_deadlock_udpmc_test WORKING_DIRECTORY $<TARGET_PROPERTY:pstm_deadlock_udpmc_test,CONTAINER_LOC>)
setup_target_for_coverage(pstm_deadlock_udpmc_test SCAN_DIR ..)

#TCP Endpoint test is disabled because the test is not stable when running on Travis

@abroekhuis abroekhuis Jul 9, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this removed intentionally?

@Oipo Oipo Jul 9, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. If you look closely, it's trying to add a TCP test inside of the UDP block. The TCP block also adds this test, it must've somehow got duplicated.

if (ENABLE_PUBSUB_PSA_TCP_ENDPOINT_TEST)
add_test(NAME pubsub_tcp_endpoint_tests COMMAND pubsub_tcp_endpoint_tests WORKING_DIRECTORY $<TARGET_PROPERTY:pubsub_tcp_endpoint_tests,CONTAINER_LOC>)
setup_target_for_coverage(pubsub_tcp_endpoint_tests SCAN_DIR ..)
endif()

#TODO fix issues with UDPMC and reanble test again
#add_test(NAME pubsub_udpmc_tests COMMAND pubsub_udpmc_tests WORKING_DIRECTORY $<TARGET_PROPERTY:pubsub_udpmc_tests,CONTAINER_LOC>)
#setup_target_for_coverage(pubsub_udpmc_tests SCAN_DIR ..)
Expand Down