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.

Sign up for GitHub

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

Jump to bottom

Context still thinks it has an active session after Session.close() #214

Open
eirrgang opened this issue Dec 13, 2018 · 5 comments
Open

Context still thinks it has an active session after Session.close() #214

eirrgang opened this issue Dec 13, 2018 · 5 comments
Labels
bug gromacs pertains specifically to GROMACS library support. See kassonlab/gromacs-gmxapi
Projects
0.0.7.2 0.0.7.5

Comments

@eirrgang
Copy link
Collaborator

ContextImpl::launch() tries to avoid reentrance problems in libgromacs by keeping track of a session launched from it using a weak pointer, but this has increasingly led to false positives in ongoing development. We now require that a Session be close()d to force the shutdown of the mdrunner at a known point. After the call to close(), the Context should be able to launch a new session.

Before simply updating the weak pointer check and reassignment, though, I need to make sure we don't have anything silly where keeping a handle to a closed session assumes that the ContextImpl can still find that session.

This bug probably wasn't more obvious because of inconsistent Python garbage collection, but ultimately the C++ API should handle the situation better. It is related to various implementation details of WorkSpec, Context, and Session, that are due for overhaul (the subject of a growing number of other issues) but this bug should be fixed for 0.0.7 ASAP.
@eirrgang eirrgang added bug gromacs pertains specifically to GROMACS library support. See kassonlab/gromacs-gmxapi gmxapi pertains to this repository and the Python module support labels Dec 13, 2018
@eirrgang
Copy link
Collaborator Author

eirrgang commented Dec 14, 2018 •
edited

The fix for GROMACS will not be in currently tagged 0.0.7 releases, so I will use the feature named 0.0.7-bugfix-https://github.com/kassonlab/gmxapi/issues/214 to indicate the fix in the library. When not found, the Python package will attempt to work around the bug by reinitializing the gmx.core.Context API object held in the gmx.context.Context object when a session is closed.

eirrgang added a commit to eirrgang/gmxapi that referenced this issue Dec 14, 2018
@eirrgang
Make sure Context does not contain an active session when it shouldn't.
05d5206 
Referemce kassonlab#214

* Add `gmx.core.has_feature` binding to `gmxapi::Version::hasFeature`
* Check whether workaround is necessary for bug kassonlab#214
* Reassign a new `gmx.core.Context` to `gmx.context.Context._api_object`
  if bugfix not reported present in libgmxapi
@eirrgang
Copy link
Collaborator Author

eirrgang commented Dec 14, 2018 •
edited

Related: Somehow, there has been a circular reference between Context and WorkSpec since 7b17db2 in February. We never rely on Python garbage collection for anything, so this is a resource leak and could cause weird behavior when the interpreter exits, I don't think it causes any runtime problems that are noticeable or testable. However, work._context can be turned into a weak reference and the whole issue will go away with https://github.com/kassonlab/gmxapi/milestone/5

eirrgang added a commit to eirrgang/gmxapi that referenced this issue Dec 14, 2018
@eirrgang
Make sure Context does not contain an active session when it shouldn't.
7dafc56 
Referemce kassonlab#214

* Add `gmx.core.has_feature` binding to `gmxapi::Version::hasFeature`
* Check whether workaround is necessary for bug kassonlab#214
* Reassign a new `gmx.core.Context` to `gmx.context.Context._api_object`
  if bugfix not reported present in libgmxapi
@eirrgang eirrgang added this to Needs triage in 0.0.7 via automation Dec 21, 2018
@eirrgang eirrgang moved this from Needs triage to High priority in 0.0.7 Dec 21, 2018
eirrgang added a commit to eirrgang/gmxapi that referenced this issue Dec 24, 2018
@eirrgang
Make sure Context does not contain an active session when it shouldn't.
8e9df68 
Referemce kassonlab#214

* Add `gmx.core.has_feature` binding to `gmxapi::Version::hasFeature`
* Check whether workaround is necessary for bug kassonlab#214
* Reassign a new `gmx.core.Context` to `gmx.context.Context._api_object`
  if bugfix not reported present in libgmxapi
* Avoid circular reference by only allowing WorkSpec to hold weak ref
  to context.
* Update and expand testing
eirrgang added a commit to eirrgang/gmxapi that referenced this issue Dec 24, 2018
@eirrgang
Make sure Context does not contain an active session when it shouldn't.
6ece65a 
Referemce kassonlab#214

* Add `gmx.core.has_feature` binding to `gmxapi::Version::hasFeature`
* Check whether workaround is necessary for bug kassonlab#214
* Reassign a new `gmx.core.Context` to `gmx.context.Context._api_object`
  if bugfix not reported present in libgmxapi
* Avoid circular reference by only allowing WorkSpec to hold weak ref
  to context.
* Update and expand testing
@eirrgang eirrgang mentioned this issue Dec 24, 2018
Merged
@eirrgang eirrgang removed the gmxapi pertains to this repository and the Python module support label Jan 16, 2019
@eirrgang
Copy link
Collaborator Author

Updates to the Python package appear to have resolved these issues. libgmxapi may or may not receive a patch to implement the fix upstream, but I'm leaving this issue open for now as a reminder.

@eirrgang
Copy link
Collaborator Author

To do: check on the resolution of this before 0.0.7.4 patch release.

@eirrgang eirrgang removed this from High priority in 0.0.7 Jul 17, 2019
@eirrgang eirrgang added this to To do in 0.0.7.4 Jul 19, 2019
@eirrgang eirrgang removed this from To do in 0.0.7.4 Jul 30, 2019
@eirrgang eirrgang added this to To do in 0.0.7.5 via automation Jul 30, 2019
@eirrgang
Copy link
Collaborator Author

Will be resolved with https://gitlab.com/gromacs/gromacs/-/issues/4422. As part of that update, the scope of the gmxapi._gmxapi.Context lifetime will be reduced to just the block surrounding the Session lifetime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug gromacs pertains specifically to GROMACS library support. See kassonlab/gromacs-gmxapi
Projects
0.0.7.5
  
To do
0.0.7.2
  
Awaiting triage
Milestone
No milestone
Development

No branches or pull requests
1 participant
@eirrgang