Skip to content

service: Refactor connection build and service.collection field set#301

Merged
bilelmoussaoui merged 1 commit into
linux-credentials:mainfrom
warusadura:session
Sep 16, 2025
Merged

service: Refactor connection build and service.collection field set#301
bilelmoussaoui merged 1 commit into
linux-credentials:mainfrom
warusadura:session

Conversation

@warusadura

Copy link
Copy Markdown
Collaborator

For some reason, even though the call to Service.connection.set() succeeds, afterwards calls to Service.connection.get() always return None.

This fixes this issue.

@bilelmoussaoui

Copy link
Copy Markdown
Collaborator

For some reason, even though the call to Service.connection.set() succeeds, afterwards calls to Service.connection.get() always return None.

Where that happens?

@warusadura

Copy link
Copy Markdown
Collaborator Author

For some reason, even though the call to Service.connection.set() succeeds, afterwards calls to Service.connection.get() always return None.

Where that happens?

Ah, forgot to mention that.

When calling service method Lock,

thread 'tokio-runtime-worker' panicked at server/src/service.rs:450:31:
called `Option::unwrap()` on a `None` value

@bilelmoussaoui

Copy link
Copy Markdown
Collaborator

Can you get the backtrace?

@warusadura

Copy link
Copy Markdown
Collaborator Author

Can you get the backtrace?

https://paste.opensuse.org/pastes/ad2f063c8e12

@bilelmoussaoui

Copy link
Copy Markdown
Collaborator

That is not the actual fix though. If you look at https://github.com/bilelmoussaoui/oo7/blob/main/server/src/service.rs#L308-L317 we do serve the Service as part of creating the connection, which means the methods can be called at that point of time, like Lock/Unlock although the connection is not yet set on the field.

So the solution is to move the serve_at after setting the connection field.

@bilelmoussaoui

Copy link
Copy Markdown
Collaborator

cc @A6GibKm as you did the changes causing this regression

@warusadura

Copy link
Copy Markdown
Collaborator Author

Yeah, understood. Thanks @bilelmoussaoui ! Will update the pr.

warusadura added a commit to warusadura/oo7 that referenced this pull request Sep 16, 2025
This completes the service object initialization before serving it and
avoids potential None returns when accessing the connection field through
service method calls.

See: linux-credentials#301 (comment)
@warusadura warusadura changed the title server: Wrap Service.connection inside an Arc server: Serve service object after setting the connection field Sep 16, 2025
@bilelmoussaoui

Copy link
Copy Markdown
Collaborator

Have you tested your changes ? does it work as expected now?

@warusadura

Copy link
Copy Markdown
Collaborator Author

Have you tested your changes ? does it work as expected now?

Yes. No more, Option::unwrap() on a None value` error. Lock method call exists nicely.

@warusadura

Copy link
Copy Markdown
Collaborator Author

Note: this fix is not related to issues mentioned in #302

Comment thread server/src/service.rs Outdated
Comment on lines +319 to +322
.at(
oo7::dbus::api::Service::PATH.as_deref().unwrap(),
service.clone(),
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a difference in behaviour though, serve_at when called the moment of building the connection, will replace the existing instance of the served interface. Where ObjectServer::at doesn't. So basically the request_replacement no longer does what it is supposed to do.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh! I didn't know that, thanks! Let me double check on this.

@warusadura warusadura changed the title server: Serve service object after setting the connection field service: Refactor connection build and service.collection field set Sep 16, 2025
Comment thread server/src/service.rs Outdated
Comment on lines +316 to +319
.serve_at(
oo7::dbus::api::Service::PATH.as_deref().unwrap(),
service.clone(),
)?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this not done above?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah.... I can do that :)

To restrict service methods from being called before setting the
service.collection field, we set the service.collection along with the
connection creation. With this approach we can still call serve_at.

Fixes potential None returns when accessing service.collection
@bilelmoussaoui bilelmoussaoui merged commit da99ffe into linux-credentials:main Sep 16, 2025
7 checks passed
@warusadura warusadura deleted the session branch October 12, 2025 19:47
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.

2 participants