Skip to content

Fix issue with passing quit() to OMC in subsequent calls of OMCSessionZMQ - #61

Closed
ne3x7 wants to merge 2 commits into
OpenModelica:masterfrom
ne3x7:master
Closed

ne3x7 wants to merge 2 commits into
OpenModelica:masterfrom
ne3x7:master

Conversation

@ne3x7

@ne3x7 ne3x7 commented Apr 11, 2018

Copy link
Copy Markdown
Contributor

Hello! I ran into a strange problem, found its source and propose a quick fix. Would be very proud to contribute.

The problem occurs when executing multiple subsequent calls to create and close OMCSessionZMQ, for example subsequently creating and managing instances of ModelicaSystem in a loop.

The issue is that at second (and possibly further) repetitions sendExpression('quit()') deadlocks in self._omc.recv_string(). The reason for that, I believe, is improper impact of sendExpression('quit()') on OMC in the first repetition.

This is a quick fix to allow for desired functionality. The best way to fix this issue is to replace REP/REQ logic in ZMQ to PUSH/PULL logic, which doesn't require responses.

@CLAassistant

CLAassistant commented Apr 11, 2018 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@sjoelund
sjoelund self-requested a review June 6, 2018 17:01

@sjoelund sjoelund left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

First of all, the DS_Store file does not belong here. I am also a little bit unsure if the result should really be the string "Force quit" as it is different in other version of OMPython; I think returning None would be more appropriate (this is what is returned when you "don't" return in Python).

I also can't reproduce crashes, because the ZMQ connection is set to None and subsequent calls to quit() do not perform any action.

Note: Having the call to quit() do something special is a little odd to me anyway. I would have preferred to have:
with OMSessionZMQ() as om:
om.sendExpression(...)
...

Automatically closes the connection

But this is old crap...

If there would be a testcase added for this I would be happy to try to reproduce it.

@sjoelund

sjoelund commented Jun 7, 2018

Copy link
Copy Markdown
Member

I cherry-picked and fixed your commit to #67. Perhaps this fix helps with the odd behavior I saw when trying to run OMPython on our Jenkins server.

@ne3x7

ne3x7 commented Jun 7, 2018

Copy link
Copy Markdown
Contributor Author

Thanks for your response. In order to reproduce the issue, prepare a “model.mo” file with a Class class in it and then run:

from OMPython import OMModel

def worker():
    m = OMModel(“model.mo”, “Class”)
    m.simulate()

for _ in range(10):
    worker()

The issue I encounter is that worker() doesn’t return the second time.

@sjoelund

sjoelund commented Jun 7, 2018

Copy link
Copy Markdown
Member

This seems to work fine on my machine with the old master. But if it is a deadlock scenario it might just be that my machine is too fast or simply not overloaded. I'll try to add this test to the new test suite.

@ne3x7

ne3x7 commented Jun 7, 2018

Copy link
Copy Markdown
Contributor Author

This might be the case. I run it on a virtual machine with very limited specs.

sjoelund added a commit to sjoelund/OMPython that referenced this pull request Jun 7, 2018
@sjoelund

sjoelund commented Jun 7, 2018

Copy link
Copy Markdown
Member

OK, I added some tests. They don't deadlock because I have quite many fixes for weird behavior of quitting the OM process, including yours which you say fixed the problem as well.

@sjoelund sjoelund closed this in #67 Jun 8, 2018
sjoelund added a commit that referenced this pull request Jun 8, 2018
The tests are based on #61
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.

3 participants