Changing getConnectionToNode structure, removing ForwardProxy.openConnection from NodeConnection
#284
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Changed However, the warnings: These are new to me - I don't remember seeing these before. I also ran the tests on the commit before my changes, and the warnings persisted at that commit too. Anyway, it just means that the warnings weren't from these changes. |
|
So after removing |
|
Rebased on master. Tests all passing after the rebase (reverted the |
joshuakarp
force-pushed
the
nodeconnection-objectmap
branch
from
ff3d6b5
to
2e277ee
Compare
|
The prior issues here #284 (comment) are all related to attempting to connect to an "offline" node (or rather, an IP:port that can't be connected to for some reason). It wasn't immediately clear why the errors were arising in only this test, so in the process of fixing this, I'm also adding an |
|
Some logging output: A few things I've realised:
// ForwardProxy
protected handleConnect = async (
...
try {
await this.connect(
nodeId,
ingressHost,
ingressPort,
clientSocket,
timer,
);
} catch (e) {
this.logger.info('caught e in forwardProxy ' + e.toString());
if (e instanceof networkErrors.ErrorConnection) {
this.logger.info('e instanceof ErrorConnection');
if (!clientSocket.destroyed) {
// Reaches here, destroys the socket, and returns
await clientSocketEnd('HTTP/1.1 400 Bad Request\r\n' + '\r\n');
clientSocket.destroy(e);
}
return;
}
|
|
@CMCDragonkai so the more targeted question... how does an error occurring in the networking domain (in this case, from the |
|
Well the exception cannot propagate because it's going over the network. So GRPC client will likely error out with its own exception. |
|
Okay. Will take a look into |
|
I'm just running a import type { Host, Port } from './src/network/types';
import Logger, { LogLevel, StreamHandler } from '@matrixai/logger';
import * as testUtils from './tests/utils';
import { makeNodeId } from './src/nodes/utils';
async function main() {
const clientLogger = new Logger('ClientLogger', LogLevel.DEBUG, [
new StreamHandler(),
]);
const client = await testUtils.setupRemoteKeynode({
logger: clientLogger,
});
const dummyNode = makeNodeId(
'vi3et1hrpv2m2lrplcm7cu913kr45v51cak54vm68anlbvuf83ra0',
);
await client.nodes.setNode(
dummyNode,
{
ip: '125.0.0.1' as Host,
port: 55555 as Port,
}
);
await client.nodes.getConnectionToNode(dummyNode);
await testUtils.cleanupRemoteKeynode(client);
}
main();And logging output: Some observations from this: On the networking side:
On the
public async start({ ... }) {
...
const waitForReady = promisify(client.waitForReady).bind(client);
// Add the current unix time because grpc expects the milliseconds since unix epoch
timeout += Date.now();
try {
await waitForReady(timeout);
} catch (e) {
throw new grpcErrors.ErrorGRPCClientTimeout();
}
...
}
|
|
|
Curiously, I wanted to see what would happen if we converted the // Add the current unix time because grpc expects the milliseconds since unix epoch
timeout += Date.now();
this.logger.info('after timeout+=: ' + timeout);
client.waitForReady(timeout, (err: Error | undefined) => {
this.logger.info('returned');
if (err != null) {
this.logger.info('Error thrown: ' + err!.toString());
}
});This results in the following terminating execution: The callback shows the error thrown: I think I'm potentially misusing the callback here though. It's asynchronous, and we only see the |
|
Nonetheless, I couldn't figure out if there was a way to reduce the number of retries, so I'll be simply applying a finite timeout to the client connection. I'm thinking that this should be the same as the connection timeout specified in |
|
Promisify should put any errors values in the callback into exceptions thrown. That's how it works. |
|
I've changed the |
|
All As seen in the output though, there is a // grpc-js/src/http_proxy.ts
} else {
log(
LogVerbosity.ERROR,
'Failed to connect to ' +
options.path +
' through proxy ' +
proxyAddressString +
' with status ' +
res.statusCode
);
reject();
}We can potentially look at suppressing this error at another time. This might become a part of the CLI review occurring on GitLab !213 at the moment: https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/213 |
|
Similar problems with the error output. |
|
|
|
It appears logging in grpc can be controlled with a few env variables. https://stackoverflow.com/questions/60934882/nodejs-how-is-logging-enabled-for-the-grpc-grpc-js-package However I believe these are actual configuration parameters as well. I wonder if it's possible to dependency inject a logger thus be able to propagate their messages into js-logger. I'd open up an issue on grpc-node asking about this if it's not obvious from docs. |
There is a https://grpc.github.io/grpc/node/grpc.html#.setLogger__anchor // grpc-js/src/logging.ts
export const setLogger = (logger: Partial<Console>): void => {
_logger = logger;
};
export const setLoggerVerbosity = (verbosity: LogVerbosity): void => {
_logVerbosity = verbosity;
};Also able to set the verbosity of the logger there. |
|
But can it be DIed from the client or server construction? |
|
I figured out why my tests were timing out... the |
Will have to look into this. |
|
Tests:
|
|
|
|
I ran each
|
|
Given that the closure issue is only occurring locally when all |
…moved ForwardProxy.openConnection from NodeConnection.start
joshuakarp
force-pushed
the
nodeconnection-objectmap
branch
from
484b4ff
to
bcfe675
Compare
|
Squashed and ready to merge. I believe the plan is to first merge https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/213, and then this should rebase and merge after. The only lingering issue is the test closure on |
This was referenced Nov 9, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Change
getConnectionToNodeover to the correctObjectMappattern from https://gist.github.com/CMCDragonkai/f58f08e7eaab0430ed4467ca35527a42Based on our sprint meeting today, I'll also remove the
openConnectioncall fromForwardProxywhen we established aNodeConnectionand ensure we don't run into any issues.Issues Fixed
getConnectionToNodeto use the correctObjectMapstructure #282Tasks
getConnectionToNodeto matchObjectMapNodeManagertestsForwardProxy.openConnectioncall fromNodeConnection.startNodeManager+NodeConnectontestsObjectMapdiagram with new structureFinal checklist