Page MenuHomePhabricator
Differential D5684

[services] Tunnelbroker - Removing of `get` and `send` methods from API and native codebase
ClosedPublic
Actions

Authored by • max on Nov 18 2022, 11:35 AM.
Tags
None
Referenced Files
F3263903: D5684.diff
Sat, Nov 16, 12:22 AM
Unknown Object (File)
Thu, Nov 7, 5:40 AM
Unknown Object (File)
Thu, Nov 7, 5:40 AM
Unknown Object (File)
Wed, Nov 6, 9:40 PM
Unknown Object (File)
Tue, Oct 22, 4:35 PM
Unknown Object (File)
Fri, Oct 18, 10:28 PM
Unknown Object (File)
Fri, Oct 18, 1:59 PM
Unknown Object (File)
Fri, Oct 18, 1:59 PM
View All Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
atul
tomek
• jon
Commits
rCOMMd489ecfd51da: [services] Tunnelbroker - Removing of `get` and `send` methods from API and…
Summary

This diff removes deprecated API methods from the Tunnelbroker API including the proto file and calls from the native codebase.
Deprecated API methods that should be removed:

  • Send
  • Get (stream)

These methods were added prior 0.5 version and were deprecated in a favor of the single bidirectional stream.

These diff changes take place in:

  • The protobuff file and its generated ones - by removing the API calls;
  • Native C++ network client-related codebase - by removing the API-related methods;
  • Tunnelbroker Rust gRPC server - by removing the empty handlers for the old API.

Linear task: ENG-1334

Test Plan

Services and native iOS and Android builds are successful.

Diff Detail

Repository
rCOMM Comm
Branch
remove-send-get-api
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• max created this revision.Nov 18 2022, 11:35 AM
• max held this revision as a draft.
Herald added a reviewer: • jon. · View Herald TranscriptNov 18 2022, 11:35 AM
Herald added subscribers: atul, tomek, ashoat. · View Herald Transcript
• max added a parent revision: D5596: [services] Tunnelbroker - Removing the old API.Nov 18 2022, 11:36 AM
Harbormaster completed remote builds in B13576: Diff 18596.Nov 18 2022, 11:48 AM
• max retitled this revision from [services] Tunnelbroker - Removing of `get` and `send` methods from API and native to [services] Tunnelbroker - Removing of `get` and `send` methods from API and native codebase.Nov 18 2022, 11:56 AM
• max edited the summary of this revision. (Show Details)
• max edited the test plan for this revision. (Show Details)
• max edited reviewers, added: atul, tomek; removed: • jon.
Herald added a reviewer: • jon. · View Herald TranscriptNov 18 2022, 11:56 AM
• max published this revision for review.Nov 18 2022, 11:57 AM
ashoat added a comment.Nov 18 2022, 12:02 PM

I imagine we'll want to keep something like GRPCStreamHostObject around in the future... we need a JSI class that can replace WebSocket, and since it's JSI it will need to be implemented in C++.

That said, the new GRPCStreamHostObject will need to interface with Rust / tonic behind the scenes.

I'm not sure if there's some way to keep the current GRPCStreamHostObject in the codebase. If we definitely need to remove it now, I guess we can always check git history when we're bringing it back.

• max added a comment.Nov 18 2022, 12:04 PM
In D5684#168833, @ashoat wrote:

I imagine we'll want to keep something like GRPCStreamHostObject around in the future... we need a JSI class that can replace WebSocket, and since it's JSI it will need to be implemented in C++.

That said, the new GRPCStreamHostObject will need to interface with Rust / tonic behind the scenes.

I'm not sure if there's some way to keep the current GRPCStreamHostObject in the codebase. If we definitely need to remove it now, I guess we can always check git history when we're bringing it back.

I removed the GRPCStreamHostObject because its contained only deprecated API calls and reactors. Not sure do we need to keep an empty file in this case.

ashoat added a comment.Nov 18 2022, 1:06 PM

That seems reasonable. Also curious to get @tomek's perspective on it

tomek accepted this revision.Nov 21 2022, 5:01 AM
In D5684#168868, @ashoat wrote:

That seems reasonable. Also curious to get @tomek's perspective on it

Agree, this sounds reasonable, but at the same time, simply removing this instead of replacing with new implementation feels like a big step backward. We avoid keeping commented out code, so keeping unused empty files also should be avoided, but maybe we can keep a file with a couple of empty methods, as a guideline for the future implementation? But maybe we can just update (create?) a task for the new implementation and link to this diff there?

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalNetworkSingleton.cpp
24

I don't think we should remove this

native/ios/Comm.xcodeproj/project.pbxproj
1080

This change is unrelated - D5695 handles this

atul requested changes to this revision.EditedNov 21 2022, 2:44 PM

Didn't do a super thorough review, but looks close.

Requesting changes to make sure the comments/questions are addressed before it's ready to land.

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalNetworkSingleton.cpp
24

+1, is there a reason for this?

native/cpp/CommonCpp/grpc/CMakeLists.txt
39

Let's remove this newline if it was accidental to keep git history clean... feel free to leave in if it was intentional

shared/protos/tunnelbroker.proto
13–15

Should this comment be removed?

This revision now requires changes to proceed.Nov 21 2022, 2:44 PM
• max planned changes to this revision.Nov 22 2022, 11:52 AM
• max added a child revision: D5706: [native] Remove gRPC related C++ code and packages from iOS and Android.Nov 23 2022, 9:04 AM
• max mentioned this in D5596: [services] Tunnelbroker - Removing the old API.Nov 23 2022, 12:09 PM
• max updated this revision to Diff 18792.Nov 23 2022, 12:52 PM
• max marked 5 inline comments as done.

Rebasing on master and fixing a nit.

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalNetworkSingleton.cpp
24

+1, is there a reason for this?

We are removing the GlobalNetworkSingleton.cpp completely in a following D5706 because this file contains only C++ gRPC code + reactors which is removed. We shouldn't keep the empty file.
I just split the cleanup into three diffs to make the review easier because it's a lot of removing and merging.

native/cpp/CommonCpp/grpc/CMakeLists.txt
39

Let's remove this newline if it was accidental to keep git history clean... feel free to leave in if it was intentional

Fixed!

native/ios/Comm.xcodeproj/project.pbxproj
1080

This change is unrelated - D5695 handles this

This should fix by rebasing on the master.

shared/protos/tunnelbroker.proto
13–15

Should this comment be removed?

Yes, we should remove this comment as it makes sense when we had an old API with send and get and remove this. It doesn't reflect anything here now.

Harbormaster failed remote builds in B13712: Diff 18792!Nov 23 2022, 12:58 PM
• max planned changes to this revision.Nov 23 2022, 1:55 PM
• max updated this revision to Diff 18793.Nov 23 2022, 3:44 PM

Regenerating jsi.

Harbormaster completed remote builds in B13713: Diff 18793.Nov 23 2022, 3:57 PM
tomek requested changes to this revision.Nov 24 2022, 1:29 AM
tomek added inline comments.
native/cpp/CommonCpp/NativeModules/InternalModules/GlobalNetworkSingleton.cpp
24

I don't think that removing GlobalNetworkSingleton is a good idea. I explicitly pointed this out in the comment https://linear.app/comm/issue/ENG-1334#comment-673e1516. This file lets us interact with the network module using proper threads. This is crucial on iOS where we don't want to spawn new thread when receiving a notification. Even if the network module itself becomes empty, it makes sense to keep the singleton as introducing a new network module would still require that.

This revision now requires changes to proceed.Nov 24 2022, 1:29 AM
• max planned changes to this revision.Nov 28 2022, 4:28 AM
• max requested review of this revision.Nov 28 2022, 5:24 AM
• max marked an inline comment as done.
• max added inline comments.
native/cpp/CommonCpp/NativeModules/InternalModules/GlobalNetworkSingleton.cpp
24

I don't think that removing GlobalNetworkSingleton is a good idea. I explicitly pointed this out in the comment https://linear.app/comm/issue/ENG-1334#comment-673e1516. This file lets us interact with the network module using proper threads. This is crucial on iOS where we don't want to spawn new thread when receiving a notification. Even if the network module itself becomes empty, it makes sense to keep the singleton as introducing a new network module would still require that.

We should unblock ENG-614 and we can just re-introduce new changes later. There is a possible discussion about how to handle the threads in a native and using a Rust network library and this will block the current work. To make this discussion I've picked out ENG-1333 and we can go forward there.
I don't think removing empty methods now and introducing new ones later instead of making changes will add pressure on reviewers (I'll link the description to the discussion).

tomek accepted this revision.Nov 28 2022, 10:16 AM
atul accepted this revision.Nov 28 2022, 4:08 PM
This revision is now accepted and ready to land.Nov 28 2022, 4:08 PM
• max updated this revision to Diff 18914.Nov 29 2022, 2:45 AM
• max marked an inline comment as done.

Rebase and merge.

Harbormaster completed remote builds in B13821: Diff 18914.Nov 29 2022, 2:58 AM
Closed by commit rCOMMd489ecfd51da: [services] Tunnelbroker - Removing of `get` and `send` methods from API and… (authored by • max). · Explain WhyNov 29 2022, 3:03 AM
This revision was automatically updated to reflect the committed changes.
• max added a commit: rCOMMd489ecfd51da: [services] Tunnelbroker - Removing of `get` and `send` methods from API and….

Revision Contents
Changeset List

Diff 18596

View Options

native/cpp/CommonCpp/NativeModules/CommCoreModule.h

Loading...
View Options

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp

Loading...
View Options

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalNetworkSingleton.cpp

Loading...
View Options

native/cpp/CommonCpp/NativeModules/InternalModules/NetworkModule.h

Loading...
View Options

native/cpp/CommonCpp/NativeModules/InternalModules/NetworkModule.cpp

Loading...
View Options

native/cpp/CommonCpp/_generated/NativeModules.h

Loading...
View Options

native/cpp/CommonCpp/_generated/NativeModules.cpp

Loading...
View Options

native/cpp/CommonCpp/grpc/CMakeLists.txt

Loading...
View Options

native/cpp/CommonCpp/grpc/Client.h

Loading...
View Options

native/cpp/CommonCpp/grpc/Client.cpp

Loading...
View Options

native/cpp/CommonCpp/grpc/ClientGetReadReactor.h

Loading...
View Options

native/cpp/CommonCpp/grpc/ClientGetReadReactor.cpp

Loading...
View Options

native/cpp/CommonCpp/grpc/GRPCStreamHostObject.h

Loading...
View Options

native/cpp/CommonCpp/grpc/GRPCStreamHostObject.cpp

Loading...
View Options

native/ios/Comm.xcodeproj/project.pbxproj

Loading...
View Options

services/tunnelbroker/src/server/mod.rs

Loading...
View Options

shared/protos/_generated/tunnelbroker.grpc.pb.h

Loading...
View Options

shared/protos/_generated/tunnelbroker.grpc.pb.cc

Loading...
View Options

shared/protos/_generated/tunnelbroker.pb.h

Loading...
View Options

shared/protos/_generated/tunnelbroker.pb.cc

Loading...
View Options

shared/protos/tunnelbroker.proto

Loading...