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

Why the port must be dropped when the context is dropped? #48

Open
sgkim126 opened this issue Jun 28, 2020 · 6 comments
Open

Why the port must be dropped when the context is dropped? #48

sgkim126 opened this issue Jun 28, 2020 · 6 comments
Labels
question Further information is requested

Comments

@sgkim126
Copy link

Arc::try_unwrap(self.port.take().unwrap()).unwrap().shutdown();

Currently, it fails to drop the context when the port is still held by others. But, there is no way to guarantee whether all other holders released before the context dropped.
How about dropping context calls shutdown and making the methods of the Port returns a Result?
It also should make the shutdown of the Port takes the mutable reference of the self. It means the user can access the port after shutdown, but I think this would be safer.

@sgkim126 sgkim126 added the question Further information is requested label Jun 28, 2020
@majecty
Copy link
Contributor

majecty commented Jun 29, 2020

there is no way to guarantee whether all other holders released before the context dropped.
Yes, it is a problem.

What I understand is that changing the signature of call, delete_request, and register to return Result. They will return Err if the port shutdown.

// Before
pub trait Port: std::fmt::Debug + Send + Sync + 'static {
    fn call(&self, packet: PacketView) -> Packet;
    fn delete_request(&self, id: ServiceObjectId);
    fn register(&self, service_object: Arc<dyn Dispatch>) -> HandleToExchange;
}
// After
pub trait Port: std::fmt::Debug + Send + Sync + 'static {
    // Return Error if port is shutdown
    fn call(&self, packet: PacketView) -> Result<Packet>;
    // Return Error if port is shutdown
    fn delete_request(&self, id: ServiceObjectId) -> Result<()>;
    // Return Error if port is shutdown
    fn register(&self, service_object: Arc<dyn Dispatch>) -> Result<HandleToExchange>;
}

It looks good to me.

@junha1
Copy link
Contributor

junha1 commented Jun 29, 2020

@majecty I think calling those methods for already shutdown port is a bug, not an error to handle.

@majecty
Copy link
Contributor

majecty commented Jun 29, 2020

@junha1 I don't understand your opinion.
What is the desired behavior of Port functions and shutdown?

@junha1
Copy link
Contributor

junha1 commented Jun 29, 2020

@majecty I thought shutdown() taking self it natural, and I didn't understand why does Seulgi thinks it should take &mut self.

@majecty
Copy link
Contributor

majecty commented Jun 29, 2020

So the original shutdown code has the problem: But, there is no way to guarantee whether all other holders released before the context dropped..
If you don't understand Seulgi's suggestion, please ask it to Seulgi.

@junha1
Copy link
Contributor

junha1 commented Jun 29, 2020

@sgkim126 If such situation (other holders have not released) happens, it should be a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants