Page MenuHomePhabricator
Differential D6624

[services] Tunnelbroker - Disabling messages persistence on messages send
ClosedPublic
Actions

Authored by • max on Feb 6 2023, 3:11 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 17, 11:39 AM
Unknown Object (File)
Thu, Apr 17, 10:29 AM
Unknown Object (File)
Thu, Apr 17, 3:45 AM
Unknown Object (File)
Thu, Apr 17, 3:29 AM
Unknown Object (File)
Wed, Apr 16, 11:15 PM
Unknown Object (File)
Wed, Apr 16, 11:07 PM
Unknown Object (File)
Wed, Apr 16, 11:36 AM
Unknown Object (File)
Wed, Apr 9, 7:00 AM
View All Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
• jon
bartek
Commits
rCOMMf95747ecac53: [services] Tunnelbroker - Disabling messages persistence on messages send
Summary

This diff introduces changes to the Tunnelbroker gRPC messages stream implementation according to the ENG-2641 task, where we need to disable messages persistence in the DynamoDB database using the config file parameter from D6622.
In this diff, we are disabling saving messages into the database when calling send messages, and a config flag from D6622 is presented.

Linear task: ENG-2641

Test Plan
  1. CI gates are passed.
  2. Manual testing: The client connects to the gRPC messages stream and sends the messages to the certain deviceID. The expected result is messages are not stored in the messages table DynamoDB database when the config flag messages.skip_persistence is presented.

Diff Detail

Repository
rCOMM Comm
Branch
disable-persistence-onsend
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• max created this revision.Feb 6 2023, 3:11 AM
• max held this revision as a draft.
Herald added a reviewer: • jon. · View Herald TranscriptFeb 6 2023, 3:11 AM
Herald added subscribers: atul, tomek, ashoat. · View Herald Transcript
• max added a parent revision: D6623: [services] Tunnelbroker - Disabling checking of the persisted messages on stream connect.Feb 6 2023, 3:12 AM
Harbormaster failed remote builds in B16170: Diff 22120!Feb 6 2023, 3:16 AM
• max updated this revision to Diff 22121.Feb 6 2023, 4:07 AM

Fixing building error.

Harbormaster completed remote builds in B16171: Diff 22121.Feb 6 2023, 4:25 AM
• max added a child revision: D6625: [services] Tunnelbroker - Extracting `deleteQueueIfEmpty` logic to a separate function.Feb 6 2023, 4:52 AM
• max edited the summary of this revision. (Show Details)Feb 6 2023, 5:27 AM
• max edited the test plan for this revision. (Show Details)
• max added a reviewer: bartek.
• max published this revision for review.Feb 6 2023, 5:45 AM
• jon requested changes to this revision.Feb 7 2023, 9:01 AM
• jon added inline comments.
services/tunnelbroker/src/libcpp/Tunnelbroker.cpp
260 ↗(On Diff #22121)

looks like a typo

This revision now requires changes to proceed.Feb 7 2023, 9:01 AM
• max planned changes to this revision.Feb 8 2023, 5:41 AM
• max updated this revision to Diff 22290.Feb 9 2023, 2:49 AM

Fixing space.

• max marked an inline comment as done.Feb 9 2023, 2:50 AM
• max added inline comments.
services/tunnelbroker/src/libcpp/Tunnelbroker.cpp
260 ↗(On Diff #22121)

looks like a typo

Thanks, @jon! It's weird that the CI was passed with this space.

Harbormaster completed remote builds in B16291: Diff 22290.Feb 9 2023, 3:03 AM
ashoat added inline comments.Feb 9 2023, 8:37 AM
services/tunnelbroker/src/libcpp/Tunnelbroker.cpp
260 ↗(On Diff #22121)

Can you investigate why? Unless this is valid Rust syntax, it sounds like something is wrong with CI. If that's the case, would be good to create a Linear task to investigate

• max marked 2 inline comments as done.Feb 10 2023, 11:13 AM
• max added inline comments.
services/tunnelbroker/src/libcpp/Tunnelbroker.cpp
260 ↗(On Diff #22121)

Can you investigate why? Unless this is valid Rust syntax, it sounds like something is wrong with CI. If that's the case, would be good to create a Linear task to investigate

I've tested it, It was built successfully locally, so this is not a CI issue.

bartek accepted this revision.Feb 10 2023, 10:16 PM
bartek added inline comments.
services/tunnelbroker/src/libcpp/Tunnelbroker.cpp
260 ↗(On Diff #22121)

Whitespaces next to :: is a valid Rust syntax. It must be valid to accept newlines and tabs when splitting very long expressions.

ashoat added inline comments.Feb 12 2023, 4:52 PM
services/tunnelbroker/src/libcpp/Tunnelbroker.cpp
260 ↗(On Diff #22121)

Ah, thanks for explaining

• jon accepted this revision.Feb 13 2023, 5:10 PM

Whitespaces next to :: is a valid Rust syntax. It must be valid to accept newlines and tabs when splitting very long expressions.

I'm used to C++ just making this into undefined behavior; a landmine for later.

This revision is now accepted and ready to land.Feb 13 2023, 5:10 PM
• max updated this revision to Diff 22979.Feb 23 2023, 4:41 AM
• max marked an inline comment as done.

Rebasing on master and parent changes.

Harbormaster completed remote builds in B16810: Diff 22979.Feb 23 2023, 4:56 AM
Closed by commit rCOMMf95747ecac53: [services] Tunnelbroker - Disabling messages persistence on messages send (authored by • max). · Explain WhyFeb 23 2023, 5:00 AM
This revision was automatically updated to reflect the committed changes.
• max added a commit: rCOMMf95747ecac53: [services] Tunnelbroker - Disabling messages persistence on messages send.

Revision Contents
Changeset List

PathSize
services/
tunnelbroker/
src/
libcpp/
8 lines

Diff 22979

View Options

services/tunnelbroker/src/libcpp/Tunnelbroker.cpp

require File.join(File.dirname(`node --print "require.resolve('expo/package.json')"`), "scripts/autolinking") require File.join(File.dirname(`node --print "require.resolve('expo/package.json')"`), "scripts/autolinking")
require File.join(File.dirname(`node --print "require.resolve('react-native/package.json')"`), "scripts/react_native_pods") require File.join(File.dirname(`node --print "require.resolve('react-native/package.json')"`), "scripts/react_native_pods")
require File.join(File.dirname(`node --print "require.resolve('@react-native-community/cli-platform-ios/package.json')"`), "native_modules") require File.join(File.dirname(`node --print "require.resolve('@react-native-community/cli-platform-ios/package.json')"`), "native_modules")
platform :ios, '12.0' platform :ios, '12.0'
require 'json' require 'json'
podfile_properties = JSON.parse(File.read('./Podfile.properties.json')) rescue {}
target 'Comm' do def common_comm_target_pods
pod 'ReactNativeKeyboardTrackingView', :path => '../../node_modules/react-native-keyboard-tracking-view'
pod 'ReactNativeKeyboardInput', :path => '../../node_modules/react-native-keyboard-input'
pod 'react-native-ffmpeg/min-lts', :podspec => '../../node_modules/react-native-ffmpeg/react-native-ffmpeg.podspec'
pod 'react-native-video/VideoCaching', :podspec => '../../node_modules/react-native-video/react-native-video.podspec'
pod 'SQLCipher-Amalgamation', :path => '../../node_modules/@commapp/sqlcipher-amalgamation' pod 'SQLCipher-Amalgamation', :path => '../../node_modules/@commapp/sqlcipher-amalgamation'
pod 'gRPC-C++', :podspec => './pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json' pod 'gRPC-C++', :podspec => './pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json'
pod 'gRPC-C++/Protobuf', :podspec => './pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json' pod 'gRPC-C++/Protobuf', :podspec => './pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json'
pod 'gRPC-Core', :podspec => './pod-patch/.patched/gRPC-Core/1.40.0/gRPC-Core.podspec.json' pod 'gRPC-Core', :podspec => './pod-patch/.patched/gRPC-Core/1.40.0/gRPC-Core.podspec.json'
pod 'Protobuf-C++', '3.15.8' pod 'Protobuf-C++', '3.15.8'
end
target 'Comm' do
pod 'ReactNativeKeyboardTrackingView', :path => '../../node_modules/react-native-keyboard-tracking-view'
pod 'ReactNativeKeyboardInput', :path => '../../node_modules/react-native-keyboard-input'
pod 'react-native-video/VideoCaching', :podspec => '../../node_modules/react-native-video/react-native-video.podspec'
pod 'react-native-ffmpeg/min-lts', :podspec => '../../node_modules/react-native-ffmpeg/react-native-ffmpeg.podspec'
common_comm_target_pods
use_expo_modules! use_expo_modules!
config = use_native_modules! config = use_native_modules!
podfile_properties = JSON.parse(File.read('./Podfile.properties.json')) rescue {}
use_react_native!( use_react_native!(
:path => config[:reactNativePath], :path => config[:reactNativePath],
# to enable hermes on iOS, change `false` to `true` and then install pods # to enable hermes on iOS, change `false` to `true` and then install pods
:hermes_enabled => podfile_properties['expo.jsEngine'] == 'hermes' :hermes_enabled => podfile_properties['expo.jsEngine'] == 'hermes'
) )
# Enables Flipper. # Enables Flipper.
# #
# Note that if you have use_frameworks! enabled, Flipper will not work and # Note that if you have use_frameworks! enabled, Flipper will not work and
# you should disable these next few lines. # you should disable these next few lines.
use_flipper!({ "Flipper-DoubleConversion" => "1.1.7" }) use_flipper!({ "Flipper-DoubleConversion" => "1.1.7" })
end
target 'NotificationService' do
common_comm_target_pods
pod 'OLMKit', :path => "../../node_modules/olm"
pod 'RCT-Folly', :podspec => "../../node_modules/react-native/third-party-podspecs/RCT-Folly.podspec"
# Lines below are only needed to compile and use Expo Secure Store in Notification Service.
# If Apple disapproves 'APPLICATION_EXTENSION_API_ONLY' flag then we can safely delete them and use
# customized Expo Secure Store that does not depend on ExpoModulesCore
podfile_properties = JSON.parse(File.read('./Podfile.properties.json')) rescue {}
use_expo_modules!
use_react_native!(
:path => "../../node_modules/react-native",
:hermes_enabled => podfile_properties['expo.jsEngine'] == 'hermes'
)
end
post_install do |installer| post_install do |installer|
ashoatUnsubmitted
Not Done
Inline Actions

Does this apply to all targets now? Are we sure we want to apply it to all targets? Is it possibly that we're inadvertedly applying it to some targets we haven't considered? (Targets that aren't the Comm app, or the notification service extension)

ashoat: Does this apply to all targets now? Are we sure we want to apply it to all targets? Is it…
marcinAuthorUnsubmitted
Done
Inline Actions

There are two changes here compared to master branch:

  1. post_install hook is moved from target 'Comm' scope to global Podfile scope
  2. some lines are added that disable APPLICATION_EXTENSION_API_ONLY flag to enable CommonCpp sources compilation in 'NotificationService' target.

Let me explain why those changes should not raise concern.
Ad. 1) According to this doc: https://www.rubydoc.info/github/CocoaPods/Core/Pod%2FPodfile%2FDSL:post_install post_install is a hook applied just before XCode project is written to disk. According to this doc: https://www.rubydoc.info/gems/cocoapods-core/0.34.1/Pod/Podfile/DSL Podfile hooks always exist in global scope. Even if this post_install hook was written inside target 'Comm' scope its behaviour would be the same as if it was in global scope. This is even more confirmed by the fact that an attempt to write multiple post_install hooks (e.g. each per target) leads to errors: https://github.com/CocoaPods/CocoaPods/issues/6172.
Ad. 2) Now that we established that we cannot have multiple post_install hooks and the one that we have should be in global scope we can admit it is impossible to use this flag anywhere else.

marcin: There are two changes here compared to master branch: 1) post_install hook is moved from target…
ashoatUnsubmitted
Not Done
Inline Actions

Thanks for the explanation!

ashoat: Thanks for the explanation!
react_native_post_install(installer) react_native_post_install(installer)
__apply_Xcode_12_5_M1_post_install_workaround(installer) __apply_Xcode_12_5_M1_post_install_workaround(installer)
# Excluding arm64 because M1 simulator isn't supported yet # Excluding arm64 because M1 simulator isn't supported yet
# Excluding 32-bit x86 because nobody uses it and it causes compilation issues # Excluding 32-bit x86 because nobody uses it and it causes compilation issues
installer.pods_project.build_configurations.each do |config| installer.pods_project.build_configurations.each do |config|
config.build_settings["EXCLUDED_ARCHS[sdk=iphonesimulator*]"] = "arm64 i386" config.build_settings["EXCLUDED_ARCHS[sdk=iphonesimulator*]"] = "arm64 i386"
ashoatUnsubmitted
Not Done
Inline Actions

It's a bit confusing to me that we exclude both of these here, but in the project.pbxproj only arm64 is mentioned. Do you know why that is?

ashoat: It's a bit confusing to me that we exclude both of these here, but in the `project.pbxproj`…
marcinAuthorUnsubmitted
Done
Inline Actions

Build settings in the XCode project refer to targets: Comm and NotificationService. This line changes build settings for pods. I did not modify this line and according to the explanation above it has alway been that we exclude both architectures for all pods. I do not know why it is that, but it is not related to any change introduced in this diff. Target 'Comm' excluded only amr64 architecture before I started to work on this issue and the very recent changes I introduced were to unify excluded architectures for both Comm and NotificationService. Would you like me to try whether we can exclude i386 arch for XCode targets as well?

marcin: Build settings in the XCode project refer to targets: Comm and NotificationService. This line…
ashoatUnsubmitted
Not Done
Inline Actions

No, that's alright – thanks for this explanation

ashoat: No, that's alright – thanks for this explanation
config.build_settings['OTHER_CPLUSPLUSFLAGS'] = '-DDONT_AUTOINSTALL_REANIMATED' config.build_settings['OTHER_CPLUSPLUSFLAGS'] = '-DDONT_AUTOINSTALL_REANIMATED'
end end
# Lines below are only needed to compile and use Expo Secure Store in Notification Service.
# If Apple disapproves 'APPLICATION_EXTENSION_API_ONLY' flag then we can safely delete them and use
# customized Expo Secure Store that does not depend on ExpoModulesCore
installer.pods_project.targets.each do |target|
target.build_configurations.each do |config|
config.build_settings['APPLICATION_EXTENSION_API_ONLY'] = 'NO'
end
end end
ashoatUnsubmitted
Not Done
Inline Actions

There's an extraneous space at the end of this line

ashoat: There's an extraneous space at the end of this line
end end