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

Reconnect socket && worker ping #600

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

Elestrias
Copy link
Collaborator

Signed-off-by: elestrias rus8-2002@mail.ru

Description of the Change

Benefits

Possible Drawbacks

Usage Examples or Tests [optional]

Alternate Designs [optional]

@Elestrias Elestrias force-pushed the RemoteWorkerPingRecconect branch 2 times, most recently from 13f02bb to 8fda0ae Compare February 17, 2022 15:04
Signed-off-by: elestrias <rus8-2002@mail.ru>
@wer1st wer1st changed the title Recconct socket && worker ping Reconnect socket && worker ping Feb 17, 2022
core/api/rpc/wsc.cpp Outdated Show resolved Hide resolved
core/api/rpc/wsc.cpp Show resolved Hide resolved
core/api/rpc/wsc.cpp Outdated Show resolved Hide resolved
core/api/rpc/wsc.cpp Outdated Show resolved Hide resolved
core/sector_storage/worker.hpp Outdated Show resolved Hide resolved
core/api/rpc/wsc.hpp Outdated Show resolved Hide resolved
Signed-off-by: elestrias <rus8-2002@mail.ru>
@codecov
Copy link

codecov bot commented Feb 18, 2022

Codecov Report

Merging #600 (48facdb) into master (7777777) will decrease coverage by 0.03%.
The diff coverage is 31.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #600      +/-   ##
==========================================
- Coverage   46.02%   45.98%   -0.04%     
==========================================
  Files         715      715              
  Lines       32328    32382      +54     
  Branches    17885    17918      +33     
==========================================
+ Hits        14880    14892      +12     
- Misses      13102    13140      +38     
- Partials     4346     4350       +4     
Impacted Files Coverage Δ
core/api/rpc/wsc.hpp 50.00% <ø> (ø)
core/api/worker_api.hpp 0.00% <ø> (ø)
core/primitives/sector_file/sector_file.hpp 80.00% <ø> (ø)
core/primitives/types.hpp 16.66% <0.00%> (-11.91%) ⬇️
core/remote_worker/remote_worker_api.cpp 0.00% <0.00%> (ø)
core/sector_storage/impl/local_worker.cpp 70.79% <0.00%> (-0.36%) ⬇️
core/sector_storage/impl/remote_worker.cpp 0.00% <0.00%> (ø)
core/sector_storage/worker.hpp 62.50% <ø> (ø)
core/api/rpc/wsc.cpp 31.91% <25.00%> (-7.03%) ⬇️
core/sector_storage/impl/scheduler_impl.cpp 69.36% <66.66%> (-0.35%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7777777...48facdb. Read the comment docs.

Signed-off-by: elestrias <rus8-2002@mail.ru>
Signed-off-by: elestrias <rus8-2002@mail.ru>
@Elestrias
Copy link
Collaborator Author

The problem I could see is that with connection retry we can connect to that socket with same host:port but it won't be the same process f e, instead of worker with gpu will be connected worker without gpu, problem is that we don't make changes in miner's worker info

Signed-off-by: elestrias <rus8-2002@mail.ru>
core/api/rpc/wsc.cpp Outdated Show resolved Hide resolved
Elestrias and others added 3 commits February 22, 2022 20:37
Signed-off-by: elestrias <rus8-2002@mail.ru>
…ct/cpp-filecoin into RemoteWorkerPingRecconect
Elestrias and others added 3 commits February 22, 2022 22:22
Signed-off-by: elestrias <rus8-2002@mail.ru>
…ct/cpp-filecoin into RemoteWorkerPingRecconect
@@ -87,27 +88,28 @@ namespace fc::api::rpc {
}
}
chans.clear();
reconnect(3, std::chrono::seconds(10));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use named variable or const instead of magic number 3

socket->async_write(boost::asio::buffer(buffer.data(), buffer.size()),
[=](auto &&ec, auto) {
std::lock_guard lock{mutex};
writing = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you change writing under the guard, but it is read and changed above without any guard. If you want to avoid race condition in multithreading, please use more consistent approach.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Moreover, you provide data by copy [=], not by refs. This change doesn't make any sense in this case.

@@ -185,4 +187,36 @@ namespace fc::api::rpc {
}
}
}

void Client::reconnect(int counter, std::chrono::milliseconds wait) {
if (reconnecting) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it thread-safe?

logger_->info(
"Starting reconnect to {}:{}", client_data.host, client_data.port);
for (int i = 0; i < counter; i++) {
std::this_thread::sleep_for(wait*(i+1));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
std::this_thread::sleep_for(wait*(i+1));
std::this_thread::sleep_for(wait * (i + 1));

@@ -6,6 +6,7 @@
#include "remote_worker/remote_worker_api.hpp"

namespace fc::remote_worker {
using api::ApiVersion;
using api::VersionResult;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not used anymore, remove

Suggested change
using api::VersionResult;

Comment on lines 910 to +911
}
void LocalWorker::ping(std::function<void(const bool resp)> cb) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
void LocalWorker::ping(std::function<void(const bool resp)> cb) {
}
void LocalWorker::ping(std::function<void(const bool resp)> cb) {

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.

None yet

4 participants