Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: Data race in new webserver #1926

Merged
merged 5 commits into from
Feb 27, 2025

Conversation

kuznetsss
Copy link
Collaborator

There was a data race inside CoroutineGroup because internal timer was used from multiple threads in the methods asyncWait() and onCoroutineComplete(). Changing registerForeign() to spawn to the same yield_context fixes the problem because now the timer is accessed only from the same coroutine which has an internal strand.

During debugging I also added websocket support for request_gun tool.

Copy link

codecov bot commented Feb 26, 2025

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.15%. Comparing base (427ba47) to head (0ed0c7d).
Report is 17 commits behind head on develop.

Files with missing lines Patch % Lines
src/web/ng/RPCServerHandler.hpp 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1926      +/-   ##
===========================================
+ Coverage    72.71%   73.15%   +0.43%     
===========================================
  Files          333      337       +4     
  Lines        13525    13819     +294     
  Branches      6881     7007     +126     
===========================================
+ Hits          9835    10109     +274     
+ Misses        1785     1783       -2     
- Partials      1905     1927      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@cindyyan317 cindyyan317 left a comment

Choose a reason for hiding this comment

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

Fix looks good to me 👍

@kuznetsss kuznetsss merged commit d11e7bc into XRPLF:develop Feb 27, 2025
21 checks passed
@kuznetsss kuznetsss deleted the Fix_crash_of_new_webserver branch February 27, 2025 15:08
PeterChen13579 pushed a commit to PeterChen13579/clio that referenced this pull request Mar 2, 2025
There was a data race inside `CoroutineGroup` because internal timer was
used from multiple threads in the methods `asyncWait()` and
`onCoroutineComplete()`. Changing `registerForeign()` to spawn to the
same `yield_context` fixes the problem because now the timer is accessed
only from the same coroutine which has an internal strand.

During debugging I also added websocket support for `request_gun` tool.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants