Page MenuHomePhabricator
Differential D4381

Implement interface for inter process non-blocking binary lock based on POSIX semaphores
ClosedPublic
Actions

Authored by marcin on Jun 28 2022, 8:32 AM.
Tags
None
Referenced Files
F5075219: D4381.id.diff
Fri, Mar 28, 3:26 PM
Unknown Object (File)
Wed, Mar 26, 6:46 PM
Unknown Object (File)
Sun, Mar 16, 8:34 AM
Unknown Object (File)
Sun, Mar 16, 8:34 AM
Unknown Object (File)
Sat, Mar 15, 7:19 PM
Unknown Object (File)
Thu, Mar 13, 8:18 PM
Unknown Object (File)
Thu, Mar 13, 8:18 PM
Unknown Object (File)
Thu, Mar 13, 8:18 PM
View All Files
Subscribers
• abosh
• adrian
ashoat
atul

Details

Reviewers
tomek
• karol
varun
Commits
rCOMM65b7c27c4c10: Implement interface for inter process non-blocking binary lock based on POSIX…
Summary

Implement methods from parent diff

Test Plan

Temporarily modify NotificationService and AppDelegate so that the first writes notification to a file from which the second reads upon app launch. Cover reads and writes with semaphore operations. Use debugger to hold semaphore in AppDelegate, trigger notification and see that NotificationService did not acquire semaphore (simply logs it).

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Jun 28 2022, 8:32 AM
Herald added subscribers: • abosh, atul, • adrian, ashoat. · View Herald TranscriptJun 28 2022, 8:32 AM
Harbormaster completed remote builds in B10056: Diff 13905.Jun 28 2022, 8:42 AM
marcin requested review of this revision.Jun 28 2022, 8:42 AM
marcin added a parent revision: D4380: Define interface for inter process non-blocking binary lock based on POSIX semaphore.Jun 29 2022, 2:34 AM
marcin added a child revision: D4382: Define interface for elementary file operations: atomic write, append, read and atomic truncation with encryption-decryption layer based on encryption key in CommSecureStore.
tomek accepted this revision.Jun 29 2022, 6:11 AM
This revision is now accepted and ready to land.Jun 29 2022, 6:11 AM
marcin updated this revision to Diff 14062.Jul 1 2022, 2:00 AM

Introduce files to project.pbxproj

Harbormaster completed remote builds in B10184: Diff 14062.Jul 1 2022, 2:12 AM
marcin edited the test plan for this revision. (Show Details)Jul 1 2022, 2:47 AM
marcin updated this revision to Diff 14149.Jul 5 2022, 3:19 AM

Add proper error handling to destroyLock method.

Harbormaster failed remote builds in B10257: Diff 14149!Jul 5 2022, 3:19 AM
marcin updated this revision to Diff 14160.Jul 5 2022, 3:29 AM

Rebase master

Harbormaster completed remote builds in B10268: Diff 14160.Jul 5 2022, 4:22 AM
ashoat added inline comments.Jul 5 2022, 11:56 AM
native/ios/Comm/TemporalMessageStorage/NonBlockingLock.mm
15–42 ↗(On Diff #14160)

Spacing here looks broken. Is this really how XCode is formatting it? Can we try to improve this?

marcin added inline comments.Jul 7 2022, 5:57 AM
native/ios/Comm/TemporalMessageStorage/NonBlockingLock.mm
15–42 ↗(On Diff #14160)

I don't use XCode for formatting - I rely on yarn clang-format-all command.

Spacing here looks broken.

What exactly do you refer to? I think you may refer to NSLocalizedDescriptionKey : [NSString. Actually spacing is correct here. Additional space is introduced between NSLocalizedDescriptionKey and : since it is not a method call but initialization of Objective - C dictionary. We do no introduce before after : if it precedes argument in method call, but we do so if it precedes value under a given key in Objective-C dictionary initialization. Example: https://stackoverflow.com/questions/9195573/initializing-nsdictionary.

ashoat added inline comments.Jul 7 2022, 6:33 AM
native/ios/Comm/TemporalMessageStorage/NonBlockingLock.mm
15–42 ↗(On Diff #14160)

Ah okay, that's fair... sorry for my confusion

marcin updated this revision to Diff 14268.Jul 7 2022, 7:05 AM

Rename Temporal to Temporary

Harbormaster completed remote builds in B10360: Diff 14268.Jul 7 2022, 7:18 AM
marcin updated this revision to Diff 14462.Jul 14 2022, 4:47 AM

Rebase

Harbormaster completed remote builds in B10513: Diff 14462.Jul 14 2022, 5:00 AM
Closed by commit rCOMM65b7c27c4c10: Implement interface for inter process non-blocking binary lock based on POSIX… (authored by marcin). · Explain WhyJul 14 2022, 5:09 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMM65b7c27c4c10: Implement interface for inter process non-blocking binary lock based on POSIX….

Revision Contents
Changeset List

PathSize
native/
ios/
Comm.xcodeproj/
6 lines
Comm/
TemporaryMessageStorage/
78 lines

Diff 14268

View Options

native/ios/Comm.xcodeproj/project.pbxproj

// @flow // @flow
import Icon from '@expo/vector-icons/FontAwesome5.js';
import { useNavigationState } from '@react-navigation/native'; import { useNavigationState } from '@react-navigation/native';
import invariant from 'invariant'; import invariant from 'invariant';
import * as React from 'react'; import * as React from 'react';
import { View } from 'react-native'; import { View, Text } from 'react-native';
import genesis from 'lib/facts/genesis.js'; import genesis from 'lib/facts/genesis.js';
import { threadInfoSelector } from 'lib/selectors/thread-selectors.js'; import { threadInfoSelector } from 'lib/selectors/thread-selectors.js';
import { import {
userInfoSelectorForPotentialMembers, userInfoSelectorForPotentialMembers,
userSearchIndexForPotentialMembers, userSearchIndexForPotentialMembers,
} from 'lib/selectors/user-selectors.js'; } from 'lib/selectors/user-selectors.js';
import { getPotentialMemberItems } from 'lib/shared/search-utils.js'; import { getPotentialMemberItems } from 'lib/shared/search-utils.js';
▲ Show 20 Lines • Show All 189 Lines • ▼ Show 20 Lines if (this.frozen) {
this.pendingListDataWithHeights = listDataWithHeights; this.pendingListDataWithHeights = listDataWithHeights;
} else { } else {
this.setState({ listDataWithHeights }); this.setState({ listDataWithHeights });
} }
}; };
} }
const unboundStyles = { const unboundStyles = {
pinnedCountBanner: {
backgroundColor: 'panelForeground',
height: 30,
flexDirection: 'row',
textAlign: 'center',
justifyContent: 'center',
alignItems: 'center',
},
pinnedCountText: {
color: 'panelBackgroundLabel',
marginRight: 5,
},
container: { container: {
backgroundColor: 'listBackground', backgroundColor: 'listBackground',
flex: 1, flex: 1,
}, },
threadContent: { threadContent: {
flex: 1, flex: 1,
}, },
hiddenThreadContent: { hiddenThreadContent: {
▲ Show 20 Lines • Show All 124 Lines • ▼ Show 20 Lines ) {
const styles = useStyles(unboundStyles); const styles = useStyles(unboundStyles);
const overlayContext = React.useContext(OverlayContext); const overlayContext = React.useContext(OverlayContext);
const measureMessages = useHeightMeasurer(); const measureMessages = useHeightMeasurer();
const genesisThreadInfo = useSelector( const genesisThreadInfo = useSelector(
state => threadInfoSelector(state)[genesis.id], state => threadInfoSelector(state)[genesis.id],
); );
const bannerText = React.useMemo(() => {
if (!threadInfo.pinnedCount || threadInfo.pinnedCount === 0) {
return '';
}
const messageNoun = threadInfo.pinnedCount === 1 ? 'message' : 'messages';
return `${threadInfo.pinnedCount} pinned ${messageNoun}`;
}, [threadInfo.pinnedCount]);
atulUnsubmitted
Not Done
Inline Actions

You don't need to use React.useMemo(...) here.

The useMemo() hook is helpful for maintaining referential equality so that objects will be considered "shallowly equal" (== in JS) and we can avoid re-renders. This is helpful for objects (including Map(), Set(), etc), arrays (which are objects), and functions (which are objects).

On the other hand, strings in JS are considered shallowly equal if they have the same contents, so we don't have to worry about re-renders if the "content" stays the same.

See below:

875403.png (620×810 px, 104 KB)

(In like C++, which you've been working w/ recently, std::strings can be allocated on the heap (unlike integers, booleans, etc) which may have been part of your reasoning that shallow equality would check reference instead of contents?)

atul: You don't need to use `React.useMemo(...)` here. The `useMemo()` hook is helpful for…
const pinnedCountBanner = React.useMemo(() => {
if (!bannerText) {
return null;
}
return (
<View style={styles.pinnedCountBanner}>
<Text style={styles.pinnedCountText}>{bannerText}</Text>
<Icon
name="chevron-right"
size={12}
color={colors.panelBackgroundLabel}
/>
</View>
);
}, [
bannerText,
styles.pinnedCountBanner,
styles.pinnedCountText,
colors.panelBackgroundLabel,
]);
return ( return (
<MessageListContextProvider threadInfo={threadInfo}> <MessageListContextProvider threadInfo={threadInfo}>
{pinnedCountBanner}
<MessageListContainer <MessageListContainer
{...props} {...props}
usernameInputText={usernameInputText} usernameInputText={usernameInputText}
updateUsernameInput={updateUsernameInput} updateUsernameInput={updateUsernameInput}
userInfoInputArray={userInfoInputArray} userInfoInputArray={userInfoInputArray}
updateTagInput={updateTagInput} updateTagInput={updateTagInput}
resolveToUser={resolveToUser} resolveToUser={resolveToUser}
otherUserInfos={otherUserInfos} otherUserInfos={otherUserInfos}
Show All 14 Lines
View Options

native/ios/Comm/TemporaryMessageStorage/NonBlockingLock.mm

// @flow // @flow
import Icon from '@expo/vector-icons/FontAwesome5.js';
import { useNavigationState } from '@react-navigation/native'; import { useNavigationState } from '@react-navigation/native';
import invariant from 'invariant'; import invariant from 'invariant';
import * as React from 'react'; import * as React from 'react';
import { View } from 'react-native'; import { View, Text } from 'react-native';
import genesis from 'lib/facts/genesis.js'; import genesis from 'lib/facts/genesis.js';
import { threadInfoSelector } from 'lib/selectors/thread-selectors.js'; import { threadInfoSelector } from 'lib/selectors/thread-selectors.js';
import { import {
userInfoSelectorForPotentialMembers, userInfoSelectorForPotentialMembers,
userSearchIndexForPotentialMembers, userSearchIndexForPotentialMembers,
} from 'lib/selectors/user-selectors.js'; } from 'lib/selectors/user-selectors.js';
import { getPotentialMemberItems } from 'lib/shared/search-utils.js'; import { getPotentialMemberItems } from 'lib/shared/search-utils.js';
▲ Show 20 Lines • Show All 189 Lines • ▼ Show 20 Lines if (this.frozen) {
this.pendingListDataWithHeights = listDataWithHeights; this.pendingListDataWithHeights = listDataWithHeights;
} else { } else {
this.setState({ listDataWithHeights }); this.setState({ listDataWithHeights });
} }
}; };
} }
const unboundStyles = { const unboundStyles = {
pinnedCountBanner: {
backgroundColor: 'panelForeground',
height: 30,
flexDirection: 'row',
textAlign: 'center',
justifyContent: 'center',
alignItems: 'center',
},
pinnedCountText: {
color: 'panelBackgroundLabel',
marginRight: 5,
},
container: { container: {
backgroundColor: 'listBackground', backgroundColor: 'listBackground',
flex: 1, flex: 1,
}, },
threadContent: { threadContent: {
flex: 1, flex: 1,
}, },
hiddenThreadContent: { hiddenThreadContent: {
▲ Show 20 Lines • Show All 124 Lines • ▼ Show 20 Lines ) {
const styles = useStyles(unboundStyles); const styles = useStyles(unboundStyles);
const overlayContext = React.useContext(OverlayContext); const overlayContext = React.useContext(OverlayContext);
const measureMessages = useHeightMeasurer(); const measureMessages = useHeightMeasurer();
const genesisThreadInfo = useSelector( const genesisThreadInfo = useSelector(
state => threadInfoSelector(state)[genesis.id], state => threadInfoSelector(state)[genesis.id],
); );
const bannerText = React.useMemo(() => {
if (!threadInfo.pinnedCount || threadInfo.pinnedCount === 0) {
return '';
}
const messageNoun = threadInfo.pinnedCount === 1 ? 'message' : 'messages';
return `${threadInfo.pinnedCount} pinned ${messageNoun}`;
}, [threadInfo.pinnedCount]);
atulUnsubmitted
Not Done
Inline Actions

You don't need to use React.useMemo(...) here.

The useMemo() hook is helpful for maintaining referential equality so that objects will be considered "shallowly equal" (== in JS) and we can avoid re-renders. This is helpful for objects (including Map(), Set(), etc), arrays (which are objects), and functions (which are objects).

On the other hand, strings in JS are considered shallowly equal if they have the same contents, so we don't have to worry about re-renders if the "content" stays the same.

See below:

875403.png (620×810 px, 104 KB)

(In like C++, which you've been working w/ recently, std::strings can be allocated on the heap (unlike integers, booleans, etc) which may have been part of your reasoning that shallow equality would check reference instead of contents?)

atul: You don't need to use `React.useMemo(...)` here. The `useMemo()` hook is helpful for…
const pinnedCountBanner = React.useMemo(() => {
if (!bannerText) {
return null;
}
return (
<View style={styles.pinnedCountBanner}>
<Text style={styles.pinnedCountText}>{bannerText}</Text>
<Icon
name="chevron-right"
size={12}
color={colors.panelBackgroundLabel}
/>
</View>
);
}, [
bannerText,
styles.pinnedCountBanner,
styles.pinnedCountText,
colors.panelBackgroundLabel,
]);
return ( return (
<MessageListContextProvider threadInfo={threadInfo}> <MessageListContextProvider threadInfo={threadInfo}>
{pinnedCountBanner}
<MessageListContainer <MessageListContainer
{...props} {...props}
usernameInputText={usernameInputText} usernameInputText={usernameInputText}
updateUsernameInput={updateUsernameInput} updateUsernameInput={updateUsernameInput}
userInfoInputArray={userInfoInputArray} userInfoInputArray={userInfoInputArray}
updateTagInput={updateTagInput} updateTagInput={updateTagInput}
resolveToUser={resolveToUser} resolveToUser={resolveToUser}
otherUserInfos={otherUserInfos} otherUserInfos={otherUserInfos}
Show All 14 Lines