Implement an API for dynamic overrides of BridgedObject attributes #4

fmagin · 2020-05-03T16:47:27Z

PR for #3

Should not break any existing functionality, unless whoever registers the overrides adds broken overrides.

It should also be possible to signal that on this specific value, jfx_bridge should handle it like there is no override.
It should still be possible to return (nearly) any value/exception from an override, so some custom exception defined in jfx_bridge would probably be the way to go. Will implement this some other day.

fmagin · 2020-05-04T15:54:52Z

@justfoxing I think I found a reliable way to detect if a callable (or object in general) is a type by checking if it has the typeName attribute. If this is good enough I can start I moving the logic in BridgedCallable into the new BridgedType.

justfoxing · 2020-05-05T07:48:23Z

Hey, Florian. I'm unsure what the point of the type changes are - they seem unrelated to the purpose of override registration. Could you pull those into a separate pull request for discussion, to make it easier to understand the changes?

justfoxing · 2020-05-05T07:50:58Z

Any chance you can generate some test cases for the register_overrides changes to give me a better sense of how you think they should be used?

fmagin · 2020-05-05T07:58:50Z

The the relation is basically that this branch is basically all the things I started changing inside jfx_bridge to build the features for ipyghidra I am currently implementing. Which is in fact a bad way to organize PRs. I will check if I can separate the changes again in some reasonable way.

If you want example/test cases, I am working on the features based on this at https://github.com/fmagin/ipyghidra/tree/dev . Tests are in https://github.com/fmagin/ipyghidra/blob/dev/tests/test_doc_helper.py and my helper registers its override at https://github.com/fmagin/ipyghidra/blob/dev/ipyghidra/doc_helper_stubfile.py#L367-L381

fmagin · 2020-05-05T08:05:47Z

What this does is that I intercept various Python attributes like __doc__, __signature__ etc and instead of traversing the bridge (which would just return an AttributeError anyway) I use mypy and a ghidra-stubs package generated with VDOO-Connected-Trust/ghidra-pyi-generator#6 to return reasonable results. Having dedicated BridgedTypes will make my code cleaner (though so far I don't use them yet) and transmitting the full type means I don't have to waste multiple bridge traversals for this.

Another thing I am thinking about implementing is some batch_get so I can grab a list of attributes with one traversal and optionally register them as overrides. Latency is critical in IPython, especially for completions, so I want to keep the number of bridge traversals to an absolute minimum. Maybe even the possibility for an external tool to register a set of attributes that are fetched on each object creation and immediately registered as overrides. E.g. __name__ and similar.

justfoxing · 2020-05-05T08:29:32Z

Yeah, pull the types stuff into a separate PR, we'll discuss it there. I've got a bunch more questions about register_overrides, so I'd like to just focus on that.

justfoxing · 2020-05-05T08:43:52Z

jfx_bridge/bridge.py

@@ -110,6 +110,12 @@ class ThreadingTCPServer(socketserver.ThreadingMixIn, socketserver.TCPServer):
 GLOBAL_BRIDGE_SHUTDOWN = False


+_EXTERNAL_OVERRIDES = {}
+
+def register_overrides(overrides):


So I generally like this API. It needs a little more documentation, but I think it makes sense.

However, I think it'd work a lot better as part of the BridgeConn object, and also exposed in the BridgeClient object (the same way remote_eval is exposed by BridgeClient). That means that you'd be able to register_overrides for a specific bridge instance - I've got a couple of use cases where I'll have multiple bridges of different types running at the same time, and I don't think you'd always want the same overrides to affect all the bridges.

justfoxing · 2020-05-05T08:55:34Z

jfx_bridge/bridge.py

@@ -1163,7 +1169,10 @@ def __init__(self, bridge_conn, obj_dict):
        self._bridge_overrides = dict()

    def __getattribute__(self, attr):
-        if attr.startswith(BRIDGE_PREFIX) or attr == "__class__" or attr in BridgedObject._DONT_BRIDGE or attr in BridgedObject._LOCAL_METHODS or (attr in BridgedObject._DONT_BRIDGE_UNLESS_IN_ATTRS and attr not in self._bridge_attrs):
+
+        if attr in _EXTERNAL_OVERRIDES:


Per the comment above, this would need to change to getting _EXTERNAL_OVERRIDES from the self._bridge_conn object.

That in turn means a little bit of playing around with the ordering of the attribute resolution - if attr.startswith(BRIDGE_PREFIX) would need to be pulled out separately so it always runs first, so you can get the self._bridge_conn without going into a loop.

This means you wouldn't be able to override any of the _bridge_ prefix attributes - but I think that's probably actually okay. I like the idea of avoiding people breaking core bridge functionality with overrides.

In my opinion this API should come with a giant warning that you should really now what you are doing before using it anyway. You could probably still end up with some loops if your override accesses some attribute that is also overridden. Probably as soon as some attribute is a reference to the object itself, but that is the issue of the override IMO.

But not allowing to overwrite BRIDGE_PREFIX attributes seems sensible, to make this cleaner. The core goal of this is to make the object that is being proxied more convincing to any attempts of introspection, so BRIDGE_PREFIX attributes are irrelevant for that. For example I would still want to override attributes like __class__ . The override could involve hacky stack frame introspection to return a different object if IPython is asking, and just concede for anything else.

Will change this accordingly.

justfoxing · 2020-05-05T09:15:37Z

jfx_bridge/bridge.py

@@ -1128,6 +1128,10 @@ def __get__(self, instance, owner):
        return functools.partial(remote_method, instance)


+class ConcedeBridgeOverrideException(Exception):


I like this concept, but prefer a shorter, clearer name. Maybe something like "BridgeOverrideFallback", or "DontUseBridgeOverride". Don't see a need for the Exception suffix - like StopIteration, this exception is not actually an exceptional condition, just a way of doing control flow.

I called it ConcedeOverride and inherited from BaseException "since it is technically not an error." which is a justification used for exceptions like GeneratorExit. I feel like the meaning of "Concede" fits really well with the meaning of "override".

This also means an override can have a generic except Exception: clause inside it to catch all unintended internal errors which shouldn't be propagated to the caller, unlike intentional Exceptions

justfoxing · 2020-05-06T21:29:36Z

jfx_bridge/bridge.py

+        try:
+            # Try calling a registered override
+            return self._bridge_conn.external_overrides[attr](self)
+        except ConcedeOverride:
+            # The override signalled that it does not want to override this case
+            pass
+        except KeyError:
+            # There was no override for this attribute
+            pass


Suggested change

try:

# Try calling a registered override

return self._bridge_conn.external_overrides[attr](self)

except ConcedeOverride:

# The override signalled that it does not want to override this case

pass

except KeyError:

# There was no override for this attribute

pass

if attr in self._bridge_conn.external_overrides:

try:

# Try calling a registered override

return self._bridge_conn.external_overrides[attr](self)

except ConcedeOverride:

# The override signalled that it does not want to override this case

pass

This way, KeyError exceptions from buggy overrides won't be hidden - the only "exception" that should be allowed is ConcedeOverride

Oh, good call, didn't realize that this changed the behavior if an override wants to return KeyError. Thanks!

justfoxing · 2020-05-06T21:29:59Z

jfx_bridge/bridge.py

@@ -1318,3 +1361,6 @@ def __next__(self):
 class BridgedIterableIterator(BridgedIterator, BridgedIterable):
    """ Common enough that iterables return themselves from __iter__ """
    pass
+
+class BridgedType(BridgedCallable):


Left in from the type stuff.

justfoxing · 2020-05-06T21:32:55Z

jfx_bridge/bridge.py

@@ -916,6 +926,8 @@ def handle_command(self, message_dict):
        self.logger.debug("Responding with {}".format(response_dict))
        return json.dumps(response_dict).encode("utf-8")

+    def register_overrides(self, overrides):


Also should have some way to remove overrides. Perhaps:

def remove_override(self, overridden_attr_name): if overridden_attr_name in self.external_overrides: del self.external_overrides[overridden_attr_name]

justfoxing reviewed May 5, 2020

View reviewed changes

fmagin mentioned this pull request May 5, 2020

Type improvements #5

Draft

Implement override interface (incl PR feedback)

26ed604

fmagin force-pushed the ipyghidra_stubs branch from 5c3c021 to 26ed604 Compare May 5, 2020 10:41

fmagin added 2 commits May 5, 2020 12:56

Add Override feature to README

b5e23f0

Add documentation to external register_overrides API

6aaa7e6

fmagin changed the title ~~WIP: Better usability for external code building up on jfx_bridge~~ Better usability for external code building up on jfx_bridge May 5, 2020

fmagin requested a review from justfoxing May 5, 2020 11:03

fmagin changed the title ~~Better usability for external code building up on jfx_bridge~~ Implement an API for dynamic overrides of BridgedObject attributes May 5, 2020

justfoxing reviewed May 6, 2020

View reviewed changes

justfoxing force-pushed the master branch 4 times, most recently from e1231a9 to a1317b6 Compare July 14, 2020 05:52

justfoxing force-pushed the master branch 3 times, most recently from a298e1b to 0658bd4 Compare February 14, 2022 10:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement an API for dynamic overrides of BridgedObject attributes #4

Implement an API for dynamic overrides of BridgedObject attributes #4

fmagin commented May 3, 2020

fmagin commented May 4, 2020

justfoxing commented May 5, 2020

justfoxing commented May 5, 2020

fmagin commented May 5, 2020

fmagin commented May 5, 2020

justfoxing commented May 5, 2020

justfoxing May 5, 2020

justfoxing May 5, 2020

fmagin May 5, 2020

justfoxing May 5, 2020

fmagin May 5, 2020

justfoxing May 6, 2020

fmagin May 7, 2020

justfoxing May 6, 2020

justfoxing May 6, 2020 •

edited

Loading

		@@ -1128,6 +1128,10 @@ def __get__(self, instance, owner):
		return functools.partial(remote_method, instance)


		class ConcedeBridgeOverrideException(Exception):

Implement an API for dynamic overrides of BridgedObject attributes #4

Are you sure you want to change the base?

Implement an API for dynamic overrides of BridgedObject attributes #4

Conversation

fmagin commented May 3, 2020

fmagin commented May 4, 2020

justfoxing commented May 5, 2020

justfoxing commented May 5, 2020

fmagin commented May 5, 2020

fmagin commented May 5, 2020

justfoxing commented May 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justfoxing May 6, 2020 • edited Loading

Choose a reason for hiding this comment

justfoxing May 6, 2020 •

edited

Loading