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

Verify/Fix Resource Management #35

Open
rneswold opened this issue Jun 16, 2021 · 5 comments
Open

Verify/Fix Resource Management #35

rneswold opened this issue Jun 16, 2021 · 5 comments
Assignees
Labels
discussion issue requires discussion
Milestone

Comments

@rneswold
Copy link
Collaborator

Issue #34 brings up a point that we may have been cleaning up our resources in the wrong location. This issue was created to track either fixing the problem or verifying it has the proper behavior. Mostly I want to verify correct behavior for the 1.x branch since that's the future of this package. These are the specifications we should meet:

  • async with DPMContext() will create a connection to a DPM. The state of that connection is maintained by the object returned in the statement (typically we call it dpm). Once this block is exited, all DPM resources are freed.
  • async for ... starts delivering pending and new packets from DPM. If this loop is exited, DPM acquisition should stop. Maybe entering an async for-loop should do an underlying dpm.start() and exiting should do dpm.stop()? At any rate, leaving the loop shouldn't harm the connection state.
  • When dpm.stop() is called, the STOP command sent to DPM must have a reply so we know DPM won't send anymore data. Then the library should clear out any pending data so that when a new async for-loop is started, the data returned is guaranteed to be new.

This allows a script to enter an acquisition loop, leave it to do other stuff, and then enter another one. The DPM flow should start and stop at the appropriate time.

@beauremus
Copy link
Contributor

Maybe it makes more sense to users to be able to manually start and stop the DPM connection. I think the context is still useful. We can require the use of the context to access self but exiting the context won't close the connection. This would allow users to reenter the context at a later time. They would have the option to manually stop the connection if that made sense. We could clean up the connection on program exit rather than context exit.

I haven't thought this through thoroughly.

@rneswold
Copy link
Collaborator Author

Python's with-statements are barriers which guarantee proper resource management (C++ uses destructors to do this because the language guarantees that when an object goes out of scope, its destructor is immediately invoked.) Python objects are garbage collected so you can't use object lifetime to manage resources. Hence the with-statement.

Our DPMContext use inside a with-statement is correct and works. The semantics for the for-loop are more flexible. I think you're right in that we preserve the .start() and .stop() methods so users can have control over the connection's activity. But I also think entering and leaving the for-loop could do some state management, too.

For instance, I'm concerned that a script leaves the for-loop and does some time-consuming thing. While it's doing that, packets would be still coming in and filling memory. If you start re-reading the packet stream (inside a new for-loop) you'd have to figure out if some data is too old and should be tossed or if you're going to process everything.

We could use the for-loops as an indicator as to when acquisition starts and stops.

@rneswold rneswold self-assigned this Jun 29, 2021
@rneswold rneswold added this to the Release v1.0 milestone Jun 29, 2021
@rneswold
Copy link
Collaborator Author

A little clarification of my previous comment is in order.

But I also think entering and leaving the for-loop could do some state management, too.

We can't detect when someone leaves or enters a for-loop (it's not like a with-statement.) What we're doing here is tracking the lifetime of the async generator returned by .replies(). Python guarantees that an iterator/generator will be closed when leaving a for-loop's scope.

This means scripts can manage the connection outside of a for-loop by calling .replies() to get the generator and calling next() to get replies. The script would be in charge of calling .close() to stop incoming replies.

@rneswold
Copy link
Collaborator Author

2c1a90c is my proposed fix for this issue.

@rneswold
Copy link
Collaborator Author

v1.0.0rc4 contains this change, so people can check it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion issue requires discussion
Projects
None yet
Development

No branches or pull requests

2 participants