feat: add a fuse to speed up the case where no resource is present #59

RaisinTen · 2022-10-28T11:21:50Z

Fixes: #58
Signed-off-by: Darshan Sen raisinten@gmail.com

src/api.js

It doesn't make sense to test just the resource injection part on a program that doesn't use the runtime API by calling any of the functions inside postject-api.h. Refs: #59 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com>

It doesn't make sense to test just the resource injection part on a program that doesn't use the runtime API by calling any of the functions inside postject-api.h. Refs: #59 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com> Signed-off-by: Darshan Sen <raisinten@gmail.com>

postject-api.h

robertgzr

since we're trying to enable others to implement either side of the process usign tools that are not postject, we should document this.

using rfc lanuage, is this going to be a MAY? so extract implementation have to work without it, but have to consider it when it is present?

postject-api.h

dsanders11 · 2022-11-09T03:39:35Z

using rfc lanuage, is this going to be a MAY? so extract implementation have to work without it, but have to consider it when it is present?

I think the other way around - if the run-time implementation (postject-api.h for the reference one) chooses to include it, the injection side should flip the value to indicate a resource is present. If the run-time implementation does not include it, the injector should be OK with it not being there and do nothing. I don't necessarily feel strongly one way or the other on it being optional, maybe leaning toward it would be nice if it was optional.

dsanders11

Could the commit message be tweaked? This doesn't speed up resource access, it speeds up the case where there is no resource present. If anything it slows down resource access with an extra check. 😄

Also, I think it would be nice if the name of the sentinel was an option (with default value) for the injection side. I tried to make all references to the name Postject configurable so any user could customize it for various reasons.

postject-api.h

test/test.cpp

ovflowd · 2022-11-25T17:36:44Z

@RaisinTen any update here? 👀

RaisinTen · 2022-11-27T06:01:23Z

@ovflowd I was thinking about addressing the remaining comments and landing once CI starts running on this repo again, see #62.

Fixes: #58 Signed-off-by: Darshan Sen <raisinten@gmail.com>

Fixes the following error: ``` In file included from /root/project/test/test.c:5: /root/project/test/../dist/postject-api.h:38:15: error: unknown type name 'bool' 38 | static inline bool postject_has_resource() { | ^~~~ ``` Refs: https://app.circleci.com/pipelines/github/postmanlabs/postject/195/workflows/ba6a05d2-1359-4876-a165-68dea6536c2b/jobs/1551?invite=true#step-110-676 Signed-off-by: Darshan Sen <raisinten@gmail.com>

Maybe that will fix this error: ``` Error: Could not find the sentinel POSTJECT_SENTINEL_fce680ab2cc467b6e072b8b5df1996b2: in the binary at inject (dist/api.js:5147:11) at async Context.<anonymous> (file:///home/circleci/project/test/cli.mjs:138:7) ``` Refs: https://app.circleci.com/pipelines/github/postmanlabs/postject/197/workflows/741145b6-e19e-4d09-b995-49eebc237962/jobs/1580 Signed-off-by: Darshan Sen <raisinten@gmail.com>

Signed-off-by: Darshan Sen <raisinten@gmail.com>

Refs: nodejs/postject#59 Signed-off-by: Darshan Sen <raisinten@gmail.com>

RaisinTen · 2023-01-05T16:06:26Z

I've imported this patch in nodejs/node#45038 and there are no related errors. PTAL everyone!

dsanders11

Overall LGTM, although I think it would be more ergonomic if the :0 suffix was internalized as an implementation detail and only POSTJECT_SENTINEL_fce680ab2cc467b6e072b8b5df1996b2 was exposed to users. Then there wouldn't need to be a validation check for the CLI option value - any string value would be valid. Would also help prevent issues since there is no validation on the macro side.

I believe this could be accomplished in postject-api.h with something like:

static inline bool postject_has_resource() {
  static const volatile char* sentinel = POSTJECT_SENTINEL_FUSE ":0";
  return sentinel[sizeof(POSTJECT_SENTINEL_FUSE)] == '1';
}

dsanders11 · 2023-01-07T00:37:23Z

src/cli.js

@@ -52,6 +53,11 @@ if (require.main === module) {
      "Name for the Mach-O segment",
      "__POSTJECT"
    )
+    .option(
+      "--sentinel-fuse <sentinel_fuse>",
+      "Sentinel fuse",


Could this be expanded to state the intended usage is for resource presence detection? I also wonder if the option name should be less generic, in case a different sentinel value is added in the future.

Done! I'm open to better names if you have a suggestion

Refs: #59 (review) Refs: #59 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com>

dsanders11

@RaisinTen, changes look good (thanks!), but the tests still have :0 in their usages. Once that's cleaned up I'm good to merge.

Refs: #59 (review) Signed-off-by: Darshan Sen <raisinten@gmail.com>

RaisinTen · 2023-01-09T14:04:25Z

@dsanders11 Good catch, done! Been a while since we released stuff. I'm probably gonna release 1.0.0-alpha.4 tomorrow morning my time, after this gets merged if that sounds good to you.

dsanders11 · 2023-01-09T14:32:49Z

@RaisinTen, sounds good to me. Merging this PR is blocked by the old CircleCI status check being required, and I don't have perms to remove (or override) that check so I'll defer to you on merging, but LGTM. 👍

RaisinTen requested review from dsanders11, jviotti and robertgzr as code owners October 28, 2022 11:21

RaisinTen mentioned this pull request Oct 28, 2022

Add a fuse to speed up resource access #58

Closed

RaisinTen commented Oct 28, 2022

View reviewed changes

src/api.js Outdated Show resolved Hide resolved

RaisinTen mentioned this pull request Oct 28, 2022

test: remove Node.js specific tests #61

Merged

RaisinTen force-pushed the feat/add-a-fuse-to-speed-up-resource-access branch from a61f9a9 to f8bb078 Compare October 28, 2022 14:26

jviotti approved these changes Oct 28, 2022

View reviewed changes

jviotti reviewed Oct 28, 2022

View reviewed changes

postject-api.h Show resolved Hide resolved

RaisinTen mentioned this pull request Oct 31, 2022

src: add initial support for single executable applications nodejs/node#45038

Merged

robertgzr reviewed Nov 3, 2022

View reviewed changes

tony-go reviewed Nov 4, 2022

View reviewed changes

postject-api.h Outdated Show resolved Hide resolved

dsanders11 requested changes Nov 9, 2022

View reviewed changes

postject-api.h Outdated Show resolved Hide resolved

test/test.cpp Outdated Show resolved Hide resolved

test/test.cpp Outdated Show resolved Hide resolved

RaisinTen added 5 commits January 5, 2023 11:42

feat: add a fuse to speed up the case where no resource is present

fcc413c

Fixes: #58 Signed-off-by: Darshan Sen <raisinten@gmail.com>

fix: address review comments

320e195

Signed-off-by: Darshan Sen <raisinten@gmail.com>

docs: add note about sentinel string for other implementers

a8c52d6

Signed-off-by: Darshan Sen <raisinten@gmail.com>

RaisinTen changed the title ~~feat: add a fuse to speed up resource access~~ feat: add a fuse to speed up the case where no resource is present Jan 5, 2023

RaisinTen force-pushed the feat/add-a-fuse-to-speed-up-resource-access branch from f8bb078 to a8c52d6 Compare January 5, 2023 06:23

feat: support overriding the fuse string

cde382d

Signed-off-by: Darshan Sen <raisinten@gmail.com>

RaisinTen requested a review from dsanders11 January 5, 2023 10:42

RaisinTen added a commit to RaisinTen/node that referenced this pull request Jan 5, 2023

test: import fuse patch

d398c7a

Refs: nodejs/postject#59 Signed-off-by: Darshan Sen <raisinten@gmail.com>

dsanders11 approved these changes Jan 7, 2023

View reviewed changes

RaisinTen added a commit that referenced this pull request Jan 9, 2023

fix: address review comments

1adb4cd

Refs: #59 (review) Refs: #59 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com>

fix: address review comments

6da6e04

Refs: #59 (review) Refs: #59 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com>

RaisinTen force-pushed the feat/add-a-fuse-to-speed-up-resource-access branch from 1adb4cd to 6da6e04 Compare January 9, 2023 11:34

RaisinTen requested a review from dsanders11 January 9, 2023 11:56

dsanders11 reviewed Jan 9, 2023

View reviewed changes

test: remove :0 from sentinel strings

8afc58b

Refs: #59 (review) Signed-off-by: Darshan Sen <raisinten@gmail.com>

RaisinTen merged commit f6b171c into main Jan 10, 2023

RaisinTen deleted the feat/add-a-fuse-to-speed-up-resource-access branch January 10, 2023 06:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add a fuse to speed up the case where no resource is present #59

feat: add a fuse to speed up the case where no resource is present #59

RaisinTen commented Oct 28, 2022

robertgzr left a comment

dsanders11 commented Nov 9, 2022

dsanders11 left a comment

ovflowd commented Nov 25, 2022

RaisinTen commented Nov 27, 2022

RaisinTen commented Jan 5, 2023

dsanders11 left a comment •

edited

dsanders11 Jan 7, 2023

RaisinTen Jan 9, 2023

dsanders11 left a comment

RaisinTen commented Jan 9, 2023

dsanders11 commented Jan 9, 2023

feat: add a fuse to speed up the case where no resource is present #59

feat: add a fuse to speed up the case where no resource is present #59

Conversation

RaisinTen commented Oct 28, 2022

robertgzr left a comment

Choose a reason for hiding this comment

dsanders11 commented Nov 9, 2022

dsanders11 left a comment

Choose a reason for hiding this comment

ovflowd commented Nov 25, 2022

RaisinTen commented Nov 27, 2022

RaisinTen commented Jan 5, 2023

dsanders11 left a comment • edited

Choose a reason for hiding this comment

dsanders11 Jan 7, 2023

Choose a reason for hiding this comment

RaisinTen Jan 9, 2023

Choose a reason for hiding this comment

dsanders11 left a comment

Choose a reason for hiding this comment

RaisinTen commented Jan 9, 2023

dsanders11 commented Jan 9, 2023

dsanders11 left a comment •

edited