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

[FIX] Use process.run instead of process.Popen, to avoid deadlock #208

Merged
merged 2 commits into from
Jun 15, 2023

Conversation

alfh
Copy link
Contributor

@alfh alfh commented Jun 1, 2023

This PR…

Considerations and implementations

The current use of subprocess.Popen always causes a hang due to deadlock when generating the 133/75 tile, for example.
The hang is when running the osmosis command, which produces quite a bit of output for this tile.

See idential issue: https://stackoverflow.com/questions/39477003/python-subprocess-popen-hanging

How to test

  1. python -m wahoomc cli -xy 133/75 -nbc -z
  2. With the changes, observe that it works fine. Without the changes, it will get stuck on "Creating .map files for tiles" step for the tile.

Pull Request Checklist

Copy link
Owner

@treee111 treee111 left a comment

Choose a reason for hiding this comment

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

wow, great PR for fixing the issue, thanks 👍

I'd love to get the suggested changes implement to have a nicer output.
And I will further test around this PR a little. Furthermore, I'll have a look why I did it the way it was and if the change supports that "why".

I'm looking forward getting this merged if we both are OK with it :-)

wahoomc/osm_maps_functions.py Outdated Show resolved Hide resolved
wahoomc/osm_maps_functions.py Outdated Show resolved Hide resolved
wahoomc/osm_maps_functions.py Outdated Show resolved Hide resolved
wahoomc/osm_maps_functions.py Outdated Show resolved Hide resolved
wahoomc/osm_maps_functions.py Outdated Show resolved Hide resolved
@treee111 treee111 changed the title Use process.run instead of process.Popen, to avoid deadlock [FIX] Use process.run instead of process.Popen, to avoid deadlock Jun 4, 2023
@treee111
Copy link
Owner

treee111 commented Jun 4, 2023

I did some research because I had in mind that i used subprocess.run() before.
I'm also no Python pro so more or less trial and error with some common programming skills built up this repo ;-)

This is the PR which changed from subprocess.run() to subprocess.Popen: #101
And this the commit: 1f7d340

Documentation is not tooo heavy at this point but if I remember right the reason was to get the output from the subprocess into Python and be able to log it. For when running in verbose mode or getting a error (in the subprocess).

Before this PR, I was just issuing messages defined by me instead of what came from the subprocess.
I remember it was kind of a hassle to get this implemented and this coding sequence has seen some changes afterwards as well.

I like your way of doing it, looks cleaner and I think we get what we want from the subprocess. Have only checked for successful calls up to now, the stderr is also filled sometimes so I am positive that it will also work where a real error occurs. As that is the most important step here I might think of a situation where a subprocess fails to get this tested (even then it might be different depending on the called subprocess osmosis, lzma, ...).

PS: If you create maps for xy-coordingate -nbc is not hurting but also not not needed. I download all countries involved in this tile because on requested the whole tile. That's why -nbc is not doing anything when in -xy mode. :-)

alfh and others added 2 commits June 15, 2023 22:20
When using process.Popen, and there is a large amount of
data being returned on std out or error by the subprocess,
it might cause a deadlock. So Popen is not recommended to
use in these casses, we should rather use the run command.
see suggestions from code review
@treee111 treee111 force-pushed the fix_subprocess_open_deadlock branch from 3e72b4a to 1b23ab5 Compare June 15, 2023 20:20
@treee111 treee111 merged commit 5e29dc1 into treee111:develop Jun 15, 2023
@treee111 treee111 linked an issue Jun 21, 2023 that may be closed by this pull request
@lucky125111
Copy link

Hi @treee111, I'm also stuck on generating 133/75 tile. Could you make a new release? If you are busy I can help with the release

@treee111
Copy link
Owner

treee111 commented Jul 10, 2023

Hi @lucky125111, thanks for the heads-up.
Here you go: https://github.com/treee111/wahooMapsCreator/releases/tag/v4.1.0

installable via
pip install wahoomc --upgrade

alfh added a commit to alfh/wahooMapsCreator that referenced this pull request Nov 4, 2023
…ee111#208)

* Use process.run instead of process.Popen, to avoid deadlock

When using process.Popen, and there is a large amount of
data being returned on std out or error by the subprocess,
it might cause a deadlock. So Popen is not recommended to
use in these casses, we should rather use the run command.

* PR review adjustments

see suggestions from code review

---------

Co-authored-by: Benjamin K <[email protected]>
@alfh alfh deleted the fix_subprocess_open_deadlock branch November 4, 2023 17:42
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.

Freeze on creating .map files for tiles (1/52)
3 participants