Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete devices #29

Merged
merged 8 commits into from
Sep 30, 2022
Merged

Delete devices #29

merged 8 commits into from
Sep 30, 2022

Conversation

Septias
Copy link
Contributor

@Septias Septias commented Sep 6, 2022

This pr adds a delete button to each device so devices no longer used can be trashed

@Septias
Copy link
Contributor Author

Septias commented Sep 6, 2022

Hey @faassen are you still actively developing this project? I've seen there has been no activity on my other pr as well for a long time. If you however find the time maybe you can look into this because I know too little about websockets & express and how to idiomatically shut down a ws-instance.
As you can see in this pr, I deleted all references but the ws still seems to live on until a new requests comes in and the ws crashes because important stuff has been deleted. This however leads to full termination of the virtual device which is what we want in the end, but it also logs an error in the console which is not so nice and I would like to fix that.

@Septias Septias requested a review from faassen September 6, 2022 07:26
backend/app.ts Show resolved Hide resolved
backend/message.ts Outdated Show resolved Hide resolved
backend/message.ts Outdated Show resolved Hide resolved
backend/instance.ts Outdated Show resolved Hide resolved
backend/message.ts Outdated Show resolved Hide resolved
backend/message.ts Outdated Show resolved Hide resolved
sim/create.ts Show resolved Hide resolved
@faassen
Copy link
Collaborator

faassen commented Sep 6, 2022

I'm not very actively working on this right now, but if you ping me here or on deltachat to remind me I can take a look once every while.

Concerning deleting devices: I'd be interested in hearing more about the use cases surrounding this - I guess it simulates a group member being removed from a group? Because intermittent connections can already be handled by disconnecting.

As a general comment, message.test.ts contains a set of tests for the underlying model. Your modifications should come with new tests that exercise the delete case.

I left some comments on your code. I am still trying to understand the behavior you describe.

@faassen
Copy link
Collaborator

faassen commented Sep 6, 2022

Oh, another small note, in your codebase "delete" comes before "add" in instance.ts which is a bit surprising if you read it.

backend/instance.ts Outdated Show resolved Hide resolved
backend/instance.ts Outdated Show resolved Hide resolved
backend/instance.ts Outdated Show resolved Hide resolved
@faassen
Copy link
Collaborator

faassen commented Sep 6, 2022

As I mentioned, using delete to wipe properties off instances isn't the way to go.

I don't really know how to close an express server either so I googled and found this:

expressjs/express#1366 (comment)

So if in instance.start you keep a reference to this on the instance, you may be able to use it to close it later. Though I think given that you send a 'delete' message from the backend you'd want to do this after this message was sent somehow.

Maybe that helps!

@Septias
Copy link
Contributor Author

Septias commented Sep 6, 2022

Thank you for this very quick response and I'm sorry you had to make so many simple corrections. This PR isn't really ready for review yet, I just pinged you because I wanted to know how to correctly shutdown ws instances.
I also pushed unfinished changes in start implementing delete Listener for you to see that I actually have some idea on how to solve the ws shutdown problem but the commit wasn't really cleaned by me, so again, sorry you had to do so much tedious reviewing. I'll change every mentioned point and work things out before requesting review again

@Septias
Copy link
Contributor Author

Septias commented Sep 6, 2022

Regarding the usecase of this, I took it out of the todos when I was looking for stuff that could be done.
Removing an instance which is no longer needed after some expressive testing seems like a reasonable use case - though it is also not a must have. What do you think?
As for what this means in webxdc space, removing the deleted device from the group with disconnect seems neccessary so I will probably implement that aswell.

@Septias
Copy link
Contributor Author

Septias commented Sep 9, 2022

I've looked into the disconnect behaviour and deleting a device is essentially a disconnect which is already covered by tests. In the end deleting a device is just a glorified disconnect so I did not add more tests

@Septias Septias changed the title [wip] Delete devices Delete devices Sep 9, 2022
@Septias Septias requested a review from faassen September 9, 2022 16:16
@Septias Septias merged commit 793f081 into main Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants