Feature/dtx service lifecycle#1625
Open
tux-mind wants to merge 16 commits into
Open
Conversation
doronz88
requested changes
Mar 17, 2026
doronz88
left a comment
doronz88
left a comment
Owner
There was a problem hiding this comment.
Overall looks great but it broke all taps. Please fix the requested change :)
|
|
||
| async def __aexit__(self, exc_type, exc_val, exc_tb): | ||
| await (await self._service_ref()).stop() | ||
| with suppress(ConnectionTerminatedError): |
Owner
There was a problem hiding this comment.
BTW, actually this piece of code should also be refactored to be like DeviceInfo service and all others, that instead of using _service_ref(), will just trigger connect only when explicitly asked or on enter. I'll fix this in a later PR. Making a service ref just to immediately stop it is funny though :P
Contributor
Author
There was a problem hiding this comment.
Yeah, today I went a bit over the Tap class and it looks like it can benefit from a refactoring.
I would make Tap a subclass of DtxService[TapService] with an @abstractmethod _getConfig(self) which subclasses would implement to provide the setConfig: argument.
I still have to fully understand the different use-cases of the received messages: when is it expected that the received notifications / data are NSKeyedArchives?
Are those 2 different types of Taps? One that automatically decodes received data using unarchive and the other that provides the messages as recevied?
DTXChannel automatically decodesnotification events with NSKeyedArchiver ( or as plist as fallback ) while data events are passed over as raw bytes.
Still, another PR as you say 😊
Contributor
Author
|
Hey, thanks for the review, let me take care of this. |
Contributor
Author
|
All good, can you please double check? |
Owner
|
Looks like taps are broken. Did not debug it myself, but you can test it: pymobiledevice3 developer dvt sysmon process monitor pid 1 |
Contributor
Author
|
Thanks for pointing that out! It should be ok now, I've tested both let me know if this looks good or needs more care 😊 |
Owner
|
Looks like the wrong exception is raised. It raises |
Contributor
Author
|
Thank you for finding that out! There was a subtle bug that polluted the exception traceback, now the original traceback is preserved and if some weird exception occurs in the dtx code you get the original traceback in the CLI. |
Owner
|
Still getting the wrong error from the same command: |
Owner
|
If you prefer, I can merge your bugfixes seperately first |
Contributor
Author
|
Hey, thanks for checking that out, for some reason I get errno 56 on my end, that's why it didn't cover the issue you're facing. The proper fix would be to isolate socket operations ( connect, read, write ) in try/catch blocks, retry the operation up to a certain number of maximum attempts and then fire a module-level exception. If you want I can take care of this improvement after April 8. Let me know if you think that other stuff is still missing from this PR, I'll find some time to improve it as you want 😊 |
Owner
|
Looks like the latest commit did not fix it: |
Owner
|
Found the bug, I'm invoking without |
This reverts commit 0d7fded.
PrimitiveInt32 and PrimitiveInt64 are signed integers, not unsigned.
Created a test which assumes that a Service is notified when the underlaying DTXConnection is closed and gracefully propagates the ConnectionTerminatedError to the Service user.
added `aclose` to handle dispose requests from various sources.
Added the ability to `dtx` to register `DTXService`s by specifying their name, allowing for reuse of the same class with different `IDENTIFIER`s. Refactored Tap to take fulll advantage of `DtxService` and async generators. Refactored sysmon and activitytrace to use the new API without major changes.
When calling the `aclose` cleanup methods on resources,
they might attempt to cleanup the very same resources that fired
the exception they have been given as cause.
In such cases, the `aclose` code might trigger a `rise exc` on
the same exception that `aclose` itself has been given ( or in general,
an exception that have been stored somewhere ).
When `raise exc` is called, python prepends the current stacktrace to
the excpetion __traceback__, polluting the stored exception ( the same object ).
In code:
```python
try:
self.connection.read(...) # original __traceback__ points here
excpetion Exception as e:
self.exception = e
with suppress(Exception):
self.connection.close()
print(e.__traceback__) # will point to self.connection.close()
```
When a device is disconnected, the error we get is `[Errno 65] No route to host` .
doronz88
force-pushed
the
feature/dtx_service_lifecycle
branch
from
ca51e64 to
d6496a4
Compare
Owner
|
After rebasing, this warning floods: |
…dServiceError` The `InvalidServiceError` exception might be misleading since we are expected to encounter it whenever the user started execution without the `--tunnel` optional even when its needed.
Owner
|
@tux-mind Added commits that fixed all issues I encountered. What do you think of them? |
Owner
|
Looks like this branch also has additional erros handling more taps.
|
Owner
|
I will still keep f754064 as this commit seems very much detached |
Owner
|
I did merge the other branch meanwhile, tackling the same issue as it currently still improves |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DTXChannel and DTXService lifecycle with callbacks.
Duplicates #1611
It also contains some minor bugfixes 😊
Let me know if it's ok or you wish to discard or re adapt it.
For community
⬇️ Please click the 👍 reaction instead of leaving a
+1or 👍 comment