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

WIP: Fixing network issues in the new feature-dep-upgrades #376

Merged
merged 4 commits into from
May 31, 2022

Conversation

CMCDragonkai
Copy link
Member

Experiment in fixing the network

@CMCDragonkai
Copy link
Member Author

@tegefaulkes this is where I'm investigating the network.

@CMCDragonkai
Copy link
Member Author

Based off: #374 (comment)

I don't think we should be cutting corners here regarding the networking, we should always ensure this is working well enough since it's the basis of all the other functionality of nodes and vaults.

@tegefaulkes
Copy link
Contributor

tegefaulkes commented May 26, 2022

Attempting to revert the version of @grpc/grpc-js from 1.6.7 to 1.3.7. This is resulting in the following build errors.

 npm run build

> @matrixai/[email protected] build
> rm -r ./dist || true; tsc -p ./tsconfig.build.json

node_modules/@grpc/grpc-js/build/src/object-stream.d.ts:11:18 - error TS2430: Interface 'IntermediateObjectWritable<T>' incorrectly extends interface 'Writable'.
  The types returned by 'end(...)' are incompatible between these types.
    Type 'void' is not assignable to type 'this'.
      'this' could be instantiated with an arbitrary type which could be unrelated to 'void'.

11 export interface IntermediateObjectWritable<T> extends Writable {
                    ~~~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/@grpc/grpc-js/build/src/server-call.d.ts:64:5 - error TS2416: Property 'end' in type 'ServerWritableStreamImpl<RequestType, ResponseType>' is not assignable to the same property in base type 'Writable'.
  Type '(metadata?: any) => void' is not assignable to type '{ (cb?: (() => void) | undefined): this; (chunk: any, cb?: (() => void) | undefined): this; (chunk: any, encoding: BufferEncoding, cb?: (() => void) | undefined): this; }'.
    Type 'void' is not assignable to type 'this'.
      'this' could be instantiated with an arbitrary type which could be unrelated to 'void'.

64     end(metadata?: any): void;
       ~~~

node_modules/@grpc/grpc-js/build/src/server-call.d.ts:77:5 - error TS2416: Property 'end' in type 'ServerDuplexStreamImpl<RequestType, ResponseType>' is not assignable to the same property in base type 'Duplex'.
  Type '(metadata?: any) => void' is not assignable to type '{ (cb?: (() => void) | undefined): this; (chunk: any, cb?: (() => void) | undefined): this; (chunk: any, encoding?: BufferEncoding | undefined, cb?: (() => void) | undefined): this; }'.
    Type 'void' is not assignable to type 'this'.
      'this' could be instantiated with an arbitrary type which could be unrelated to 'void'.

77     end(metadata?: any): void;
       ~~~


Found 3 errors in 2 files.

Errors  Files
     1  node_modules/@grpc/grpc-js/build/src/object-stream.d.ts:11
     2  node_modules/@grpc/grpc-js/build/src/server-call.d.ts:64

The problem seems to be that GRPC extends some types provided by node. and given the new version of node GRPC isn't extending the type properly anymore. Specifically the end method was changed to return this instead of `void.

This must've been the install/build errors that @emmacasolin saw.

I don't really have any evidence for or against this version change being the problem however.

@tegefaulkes
Copy link
Contributor

Found one of the problems. For the 4 tests that have the write after end error. We can fix this by changing the following.

      tlsSocket.on('end', () => {
        logger.debug('Reverse: receives tlsSocket ending');
        tlsSocketEnd();
        if (utpConn.destroyed) {
          logger.debug('Reverse: destroys tlsSocket');
          tlsSocket.destroy();
        } else {
          logger.debug('Reverse: responds tlsSocket ending');
          // Why are we ending the tlsSocket during it's end event?
          // tlsSocket.end(); // removing this removes the error. 
          utpConn.end(); // Seems like we should be ending the utpConn when the TLS ends.
          tlsSocket.destroy();
        }
      });

@tegefaulkes
Copy link
Contributor

The problem with the last test also seems to be a problem with the test itself. It seems to be caused by trying to connect to 0.0.0.0 with the UTP socket. That's why it was fixed when setting the proxyHost when starting the proxy. Otherwise it was taking the default host of '0.0.0.0'.

The fix for this is still the same. Just need to add the following to starting the proxy;

    await proxy.start({
      serverHost: serverHost(),
      serverPort: serverPort(),
      forwardHost: localHost, // Add this
      proxyHost: localHost, // Add this
      tlsConfig: {
        keyPrivatePem: keyPairPem.privateKey,
        certChainPem: certPem,
      },
    });

Theses should be added to all of the other tests just in case. We only want to bind the the localhost for testing anyway.

@tegefaulkes
Copy link
Contributor

@CMCDragonkai What were you doing in the WIP commit? so I know how to rename or squash it out.

@tegefaulkes tegefaulkes force-pushed the feature-dep-upgrades-network branch from 8b96535 to 2d78e92 Compare May 26, 2022 06:29
@CMCDragonkai
Copy link
Member Author

My original goal was to revert your changes to Proxy.test.ts, in doing so I copied over from master branch.

Then slowly reintroduce your changes as I can see incrementally what the effects are.

Some of your changes was a bit confusing, especially removing lines of code that close/end sockets or other resources. I didn't finish it.

@CMCDragonkai
Copy link
Member Author

Found one of the problems. For the 4 tests that have the write after end error. We can fix this by changing the following.

      tlsSocket.on('end', () => {
        logger.debug('Reverse: receives tlsSocket ending');
        tlsSocketEnd();
        if (utpConn.destroyed) {
          logger.debug('Reverse: destroys tlsSocket');
          tlsSocket.destroy();
        } else {
          logger.debug('Reverse: responds tlsSocket ending');
          // Why are we ending the tlsSocket during it's end event?
          // tlsSocket.end(); // removing this removes the error. 
          utpConn.end(); // Seems like we should be ending the utpConn when the TLS ends.
          tlsSocket.destroy();
        }
      });

Receiving an end event is not the same as sending an end event. The former is about receiving an FIN package, the latter is about sending a FIN packet.

If it fails to send a FIN packet, then that's because the utp connection is destroyed. But as you can see I already checked for that.

This is why I said it sounds familiar, since it's like a problem we have dealt with before.

@tegefaulkes tegefaulkes force-pushed the feature-dep-upgrades-network branch from 2d78e92 to 2f11571 Compare May 27, 2022 06:01
@tegefaulkes tegefaulkes force-pushed the feature-dep-upgrades-network branch from 2f11571 to 26173b8 Compare May 27, 2022 06:02
@tegefaulkes
Copy link
Contributor

tegefaulkes commented May 27, 2022

Should be close to merging. There is some things I need to check first.

@tegefaulkes
Copy link
Contributor

Looking over how Proxy handles the utpConn. It seems that we just don't handle any errors coming out of the utpConn. Since this problem only happens when the socket is ending anyway I think we can just safely ignore the error in the tests. Or do what was suggested and log out a warning during the test if an error happens.

@tegefaulkes tegefaulkes force-pushed the feature-dep-upgrades-network branch from 26173b8 to ac7b8dd Compare May 27, 2022 07:09
@tegefaulkes
Copy link
Contributor

This is ready to merge now.

@CMCDragonkai
Copy link
Member Author

I think you should be logging out a warning here. If you're happy to proceed, just merge it into #374. I'll come back around to reviewing the networking issue soon. Main important thing is to code to the protocol, and add adjustments when the implementation doesn't meet the protocol. The utp-native library seems flaky atm, and assuming SI transaction works well enough, we may be in a position of integrating a different network stack like QUIC #234 (as I'll have the understanding and tooling for integrating C/C++ code into our JS libraries).

@tegefaulkes tegefaulkes merged commit 03f77fb into feature-dep-upgrades May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants