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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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

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.

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.

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

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 ↗	(On Diff #18596)	I don't think we should remove this
native/ios/Comm.xcodeproj/project.pbxproj
1080 ↗	(On Diff #18596)	This change is unrelated - D5695 handles this

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 ↗	(On Diff #18596)	+1, is there a reason for this?
native/cpp/CommonCpp/grpc/CMakeLists.txt
39 ↗	(On Diff #18596)	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 ↗	(On Diff #18596)	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

Rebasing on master and fixing a nit.

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalNetworkSingleton.cpp
24 ↗	(On Diff #18596)	+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 ↗	(On Diff #18596)	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 ↗	(On Diff #18596)	This change is unrelated - D5695 handles this This should fix by rebasing on the master.
shared/protos/tunnelbroker.proto
13–15 ↗	(On Diff #18596)	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

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 ↗	(On Diff #18596)	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 ↗	(On Diff #18596)	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

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….

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 18919

native/cpp/CommonCpp/NativeModules/CommCoreModule.h

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp

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

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

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

native/cpp/CommonCpp/_generated/NativeModules.h

native/cpp/CommonCpp/_generated/NativeModules.cpp

native/cpp/CommonCpp/grpc/CMakeLists.txt

native/cpp/CommonCpp/grpc/Client.h

native/cpp/CommonCpp/grpc/Client.cpp

native/cpp/CommonCpp/grpc/ClientGetReadReactor.h

native/cpp/CommonCpp/grpc/ClientGetReadReactor.cpp

native/cpp/CommonCpp/grpc/GRPCStreamHostObject.h

native/cpp/CommonCpp/grpc/GRPCStreamHostObject.cpp

native/ios/Comm.xcodeproj/project.pbxproj

native/schema/CommCoreModuleSchema.js

services/tunnelbroker/src/server/mod.rs

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

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

shared/protos/_generated/tunnelbroker.pb.h

shared/protos/_generated/tunnelbroker.pb.cc

shared/protos/tunnelbroker.proto

[services] Tunnelbroker - Removing of `get` and `send` methods from API and native codebaseClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 18919

native/cpp/CommonCpp/NativeModules/CommCoreModule.h

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp

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

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

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

native/cpp/CommonCpp/_generated/NativeModules.h

native/cpp/CommonCpp/_generated/NativeModules.cpp

native/cpp/CommonCpp/grpc/CMakeLists.txt

native/cpp/CommonCpp/grpc/Client.h

native/cpp/CommonCpp/grpc/Client.cpp

native/cpp/CommonCpp/grpc/ClientGetReadReactor.h

native/cpp/CommonCpp/grpc/ClientGetReadReactor.cpp

native/cpp/CommonCpp/grpc/GRPCStreamHostObject.h

native/cpp/CommonCpp/grpc/GRPCStreamHostObject.cpp

native/ios/Comm.xcodeproj/project.pbxproj

native/schema/CommCoreModuleSchema.js

services/tunnelbroker/src/server/mod.rs

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

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

shared/protos/_generated/tunnelbroker.pb.h

shared/protos/_generated/tunnelbroker.pb.cc

shared/protos/tunnelbroker.proto

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

Revision Contents
Changeset List