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

Logic thread fixes #935

Merged
merged 27 commits into from Dec 3, 2019

Conversation

@jagerman
Copy link
Collaborator

jagerman commented Nov 29, 2019

Cherry picked and cleaned up from all the various fixes we applied over the last few days on a mess of branches.

@jagerman jagerman requested review from notlesh, tewinget and majestrate Nov 29, 2019
bool
TunEndpoint::ShouldFlushNow(llarp_time_t now) const
{
static constexpr llarp_time_t FlushInterval = 25;

This comment has been minimized.

Copy link
@tewinget

tewinget Nov 30, 2019

Collaborator

I wonder if this (and other constants in other files) should be at the top of the class/file so all constants for a given class are in one place. Tough call though.

auto *self = static_cast< TunEndpoint * >(tun->user);
LogicCall(self->m_router->logic(), [self]() { self->Flush(); });
auto *self = static_cast< TunEndpoint * >(tun->user);
const auto now = self->Now();

This comment has been minimized.

Copy link
@tewinget

tewinget Nov 30, 2019

Collaborator

so the libuv event loop snapshots a "now" every event loop tick that could be used here (tun->loop->whatever or something like that). idk if the current Windows event loop supports that concept though.

"holy crap, we are trying to queue a job onto the logic "
"thread but "
"it looks full");
LogErrorExplicit(TAG, LINE, "holy crap, we are trying to queue a job "

This comment has been minimized.

Copy link
@tewinget

tewinget Nov 30, 2019

Collaborator

ok, so since this got reformatted anyway, perhaps now is a good time to change the log message >_>

Copy link
Collaborator

notlesh left a comment

I noticed a couple things that are worth reviewing, but otherwise LGTM.

@@ -16,7 +16,7 @@ BUILD_TYPE ?= Debug
PYTHON ?= python
PYTHON3 ?= python3

FORMAT ?= clang-format
FORMAT ?= clang-format-8

This comment has been minimized.

Copy link
@notlesh

notlesh Dec 2, 2019

Collaborator

I think getting everyone on the same page with clang-format is a broader effort than this PR 😆

if(_stopping.load())
return;

if(_logic->numPendingJobs() >= PumpJobThreshhold
&& _lastPump + PumpInterval >= now)

This comment has been minimized.

Copy link
@notlesh

notlesh Dec 2, 2019

Collaborator

Should this be || instead of &&? As in, "either we have a lot of jobs or it's been long enough?"

This comment has been minimized.

Copy link
@tewinget

tewinget Dec 2, 2019

Collaborator

There may be a more clear way to write it, but what this is saying is "if the logic thread is a certain amount of busy and we have pumped recently enough".

@@ -763,6 +763,9 @@ namespace libuv
" has invalid fd: ", m_Device->tun_fd);
return false;
}

tuntap_set_nonblocking(m_Device, 1);

This comment has been minimized.

Copy link
@notlesh

notlesh Dec 2, 2019

Collaborator

I think this was added as a "maybe this helps", and is now more of a "probably doesn't hurt." We've probably tested this enough that the latter is a safe assumption, but for the record:

vendor/libtuntap-master/tuntap-unix.c line ~ 327 makes a call to write() that this could affect, esp. as it might return EAGAIN because of non-blocking mode.

auto ev = std::move(maybe.value());
ProtocolMessage::ProcessAsync(ev.fromPath, ev.pathid, ev.msg);
} while(true);
}

This comment has been minimized.

Copy link
@notlesh

notlesh Dec 2, 2019

Collaborator

not a big fan of do { ... } while, esp. while(true)... just sayin'

This comment has been minimized.

Copy link
@tewinget

tewinget Dec 2, 2019

Collaborator

It feels verbose/redundant to do the assignment twice (once before the loop, once at the end of each iteration), but I agree that it would be more clear to write the loop as while (maybe.has_value()).

SharedSecret shared;
if(!handler->GetCachedSessionKeyFor(T, shared))

auto v = new AsyncDecrypt();

This comment has been minimized.

Copy link
@notlesh

notlesh Dec 2, 2019

Collaborator

Smart pointer?

@majestrate majestrate merged commit b08897a into loki-project:dev Dec 3, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.