treee111 / wahooMapsCreator

Create maps for Wahoo device based on latest OSM maps
247 stars 25 forks source link

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

Closed alfh closed 1 year ago

alfh commented 1 year ago

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

treee111 commented 1 year ago

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: https://github.com/treee111/wahooMapsCreator/pull/101/commits/1f7d34004ae2f655eefef08c8eb4b54b6516265e

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. :-)

lucky125111 commented 1 year ago

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 commented 1 year ago

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