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

Fixes #1255: Introduce an activation lock to protect the raw connecti… #1262

Merged
merged 2 commits into from
Oct 23, 2023

Conversation

ganeshmurthy
Copy link
Contributor

…on from being activated when it is being torn down

@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #1262 (25c4c05) into main (f17aedb) will decrease coverage by 0.55%.
Report is 1 commits behind head on main.
The diff coverage is 90.00%.

❗ Current head 25c4c05 differs from pull request most recent head 9607aa5. Consider uploading reports for the commit 9607aa5 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1262      +/-   ##
==========================================
- Coverage   78.95%   78.41%   -0.55%     
==========================================
  Files         244      246       +2     
  Lines       63056    63568     +512     
  Branches     5931     5874      -57     
==========================================
+ Hits        49784    49844      +60     
- Misses      10616    11070     +454     
+ Partials     2656     2654       -2     
Flag Coverage Δ
pysystemtests 87.72% <ø> (+0.02%) ⬆️
pyunittests 54.60% <ø> (?)
systemtests 72.41% <90.00%> (+0.10%) ⬆️
unittests 26.52% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
calculator 30.06% <0.00%> (∅)
systemtests 79.02% <90.00%> (+0.07%) ⬆️

@@ -1410,10 +1418,12 @@ static void CORE_activate(void *context, qdr_connection_t *core_conn)

case TL_CONNECTION:
conn = (tcplite_connection_t*) common;
sys_mutex_lock(&conn->activation_lock);
if (IS_ATOMIC_FLAG_SET(&conn->raw_opened)) {
SET_ATOMIC_FLAG(&conn->core_activation);
Copy link
Member

Choose a reason for hiding this comment

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

With this lock in place, are there any atomic flags we can convert to bools?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the conn->raw_opened flag can be knocked down to boolean. The conn->core_activation flag needs to remain atomic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added code to this PR that set the conn->raw_opened to boolean. Please take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something is super wrong with the change I made. CI is blowing up. I am looking into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After further analysis, the atomic flags that are used inside the new activation lock are also sometimes used outside the activation lock, so my conclusion is that those flags cannot be converted from atomic to boolean

src/cutthrough_utils.c Outdated Show resolved Hide resolved
@ganeshmurthy ganeshmurthy merged commit bcbafec into skupperproject:main Oct 23, 2023
28 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants