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

Remove public Shutdown method on WriteHandler. #8713

Merged

Conversation

bzbarsky-apple
Copy link
Contributor

We don't ever have long-lived WriteHandler instances, so don't need
external shutdown on them.

Fixes #8651

Problem

Exposed method that can be misused.

Change overview

Stop exposing it.

Testing

Code compiles; there were no consumers of this method outside this class.

We don't ever have long-lived WriteHandler instances, so don't need
external shutdown on them.

Fixes project-chip#8651
@msandstedt msandstedt self-requested a review August 2, 2021 14:29
@msandstedt
Copy link
Contributor

We don't ever have long-lived WriteHandler instances, so don't need
external shutdown on them.

I don't understand this. Write operations take more than zero time (right?), which is why we need the interaction model to asynchronously track their state in the first place. So they will be in-flight for some finite amount of time. How then do these substantively differ from any other type of interaction?

@msandstedt msandstedt self-requested a review August 2, 2021 14:49
@bzbarsky-apple
Copy link
Contributor Author

Write operations take more than zero time (right?)

Yes...

which is why we need the interaction model to asynchronously track their state in the first place.

On the sender side. On the receiver side, the WriteHandler is created, WriteHandler::OnWriteRequest is called, and by the time it returns the WriteHandler has been shut down. Nothing async going on there.

If we add async behavior under WriteHandler::OnWriteRequest we will obviously need to change things to accommodate that.

How then do these substantively differ from any other type of interaction?

Read and command interactions currently have the ability for the responding end (CommandHandler/ReadHandler) to do async work before responding. Write interactions do not currently have such an ability: they synchronously respond to the write with a status response.

Again, this is not a feature of the spec, but just of our code. And if we change this we will want to change how the lifetime of WriteHandler is managed in various ways. But right now we could literally be allocating our WriteHandler objects on the stack, and we should strongly consider doing that....

@msandstedt
Copy link
Contributor

Write operations take more than zero time (right?)

Yes...

which is why we need the interaction model to asynchronously track their state in the first place.

On the sender side. On the receiver side, the WriteHandler is created, WriteHandler::OnWriteRequest is called, and by the time it returns the WriteHandler has been shut down. Nothing async going on there.

If we add async behavior under WriteHandler::OnWriteRequest we will obviously need to change things to accommodate that.

How then do these substantively differ from any other type of interaction?

Read and command interactions currently have the ability for the responding end (CommandHandler/ReadHandler) to do async work before responding. Write interactions do not currently have such an ability: they synchronously respond to the write with a status response.

Again, this is not a feature of the spec, but just of our code. And if we change this we will want to change how the lifetime of WriteHandler is managed in various ways. But right now we could literally be allocating our WriteHandler objects on the stack, and we should strongly consider doing that....

OK, this answers my other question about whether writes are sync. Yes they are, and this approach relies on that.

Also agree that we ought to just being doing this on the stack.

@mspang mspang merged commit 7f21452 into project-chip:master Aug 5, 2021
@bzbarsky-apple bzbarsky-apple deleted the no-writehandler-shutdown branch August 6, 2021 02:41
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
We don't ever have long-lived WriteHandler instances, so don't need
external shutdown on them.

Fixes project-chip#8651
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove public Shutdown method on WriteHandler
6 participants