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

Explicitly remove remoteIDs from the connections table where appropriate #974

Merged
merged 2 commits into from
Mar 25, 2024

Conversation

AaronH88
Copy link
Contributor

@AaronH88 AaronH88 commented Mar 20, 2024

what if a lot of time elapses between timedOut and removeconnection?
timedOutconn
s.removeConnection(conn) <---- takes a long time to delete conn
during that "takes a long time" it's possible the runProtocol exited, tcp dialed a new connection, and a new runProtocol is started (as it should!).
but then once "takes a long time" occurs, we are going to call removeconnection on that conn again! not good
this is possible because of the structure locks that removeconnection has to acquire before modifying the connections array
this would cause us to be in runProtocol, but the connections array is nowmissing our tcp connection and nothing can get routed. No redialing will occur because runProtocol is running just fine. monitoringconnectionaging won't bail us out because it also no longer sees the conn

Copied from @fosterseth

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 10.00000% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 39.64%. Comparing base (c19fdcc) to head (7451a25).

@@            Coverage Diff             @@
##            devel     #974      +/-   ##
==========================================
- Coverage   39.73%   39.64%   -0.10%     
==========================================
  Files          42       42              
  Lines        6606     6614       +8     
==========================================
- Hits         2625     2622       -3     
- Misses       3784     3792       +8     
- Partials      197      200       +3     
Files Coverage Δ
pkg/netceptor/netceptor.go 55.49% <10.00%> (-0.82%) ⬇️

... and 1 file with indirect coverage changes

@AaronH88 AaronH88 force-pushed the clean_connections_map branch 3 times, most recently from a44c24e to 199e6ab Compare March 20, 2024 14:30
@shanemcd
Copy link
Member

@AaronH88 Can you update the PR description to outline what this change does and why it's needed?

@fosterseth
Copy link
Member

seems there are more return statements we'd need to call removeconnection, e.g. https://github.com/ansible/receptor/pull/974/files#diff-23f6be7c946bcb25e244cc53843ea9dfff1ba921dfb1b2645a619c060aa6bd62R2046

I'd still prefer to do this in defer(), maybe before the if established block

Copy link

sonarcloud bot commented Mar 21, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

pkg/netceptor/netceptor.go Show resolved Hide resolved
pkg/netceptor/netceptor.go Show resolved Hide resolved
@AaronH88 AaronH88 merged commit e32df9f into ansible:devel Mar 25, 2024
15 of 16 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