curl hangs loading footprints from github

[Chris Pavlina]

Occasionally, when loading footprints from github, libcurl freezes in curl_easy_perform() waiting on data from the network (particularly on Win10 for me). Because there is no timeout, there is no way to interrupt it other than by force-quitting kicad.

Please don't immediately run and fix this - as part of my work to add footprint selection to eeschema, I've reworked the footprint loading into an asynchronous loader, and that changes the ways we can handle this. I just want to discuss possibilities:

1. Add a fixed timeout. This is easily done with curl_easy_setopt(); we'd have to decide what the timeout should be. Downside is that it's a bit ugly. I'm sure no matter what timeout we pick, someone will insist it's too short for their slow connection.

2. Add a configurable timeout. Same as #1 but we also need to make the timeout configurable - a bit difficult as the footprint loader currently has no simple way I can find to get settings to it, and because the timeout setting is not specific to one application (we currently don't have a "common" settings panel/category).

3. Add an abort function. With the asynchronous loader this is possible now. libcurl can have a progress callback, which is allowed to return a value to abort the transfer. This would require a change to the plugin API, because the command to abort has to come all the way from whatever called FP_LIB_TABLE::FootprintLoad. Implementation would be simple though: async loader objects holds an atomic_bool "m_abort_transfer"; progress callback checks it; async loader Abort() function sets m_abort_transfer true and joins the threads.

Opinions? I'm in favor of #3 but it's also the most work.

[Chris Pavlina (pavlina-chris)]

(I should note, since I mentioned reworking the footprint loader, that this is not just in my branch, I verified the bug exists in master as well.)

[Wayne Stambaugh (stambaughw)]

Option 3 would be ideal but I'm OK with option 2. Using a fixed timeout
probably doesn't make sense given the variation in the quality of users
internet connections.

We have common configuration support in KiCad. That's where the
environment variable configuration lives along with other common
settings. You just need to use "kicad_common" as the config file.

On 3/19/2017 12:18 AM, Chris Pavlina wrote:
> Public bug reported:
>
> Occasionally, when loading footprints from github, libcurl freezes in
> curl_easy_perform() waiting on data from the network (particularly on
> Win10 for me). Because there is no timeout, there is no way to interrupt
> it other than by force-quitting kicad.
>
> Please don't immediately run and fix this - as part of my work to add
> footprint selection to eeschema, I've reworked the footprint loading
> into an asynchronous loader, and that changes the ways we can handle
> this. I just want to discuss possibilities:
>
> 1. Add a fixed timeout. This is easily done with curl_easy_setopt();
> we'd have to decide what the timeout should be. Downside is that it's a
> bit ugly. I'm sure no matter what timeout we pick, someone will insist
> it's too short for their slow connection.
>
> 2. Add a configurable timeout. Same as #1 but we also need to make the
> timeout configurable - a bit difficult as the footprint loader currently
> has no simple way I can find to get settings to it, and because the
> timeout setting is not specific to one application (we currently don't
> have a "common" settings panel/category).
>
> 3. Add an abort function. With the asynchronous loader this is possible
> now. libcurl can have a progress callback, which is allowed to return a
> value to abort the transfer. This would require a change to the plugin
> API, because the command to abort has to come all the way from whatever
> called FP_LIB_TABLE::FootprintLoad. Implementation would be simple
> though: async loader objects holds an atomic_bool "m_abort_transfer";
> progress callback checks it; async loader Abort() function sets
> m_abort_transfer true and joins the threads.
>
> Opinions? I'm in favor of #3 but it's also the most work.
>
> ** Affects: kicad
> Importance: Medium
> Status: New
>
>
> ** Tags: cvpcb github pcbnew
>

[Jeff Young (jeyjey)]

I've implemented a progress reporter for the main FootprintLoad. Cancelling it generates a "Load incomplete; cancelled by user" error and joins the threads.

Does this meet the requirements of (3)?

[Wayne Stambaugh (stambaughw)]

As long as you didn't put any UI code in FP_LIB_TABLE than it sounds like it meets the criteria for #3. It's critical that we keep wx UI code out of our low level I/O objects to prevent the low level code from becoming a highly coupled mess. The reason for this is it gets really nasty trying to provide python support for the low level objects when you do this.

[Jeff Young (jeyjey)]

The UI code to handle the (optional) progress reporter is up in FOOTPRINT_LIST_IMPL, while the code to instantiate the reporter is in PCB_BASE_FRAME and CVPCB_MAINFRAME.

(The guts of the fix are attached to https://bugs.launchpad.net/kicad/+bug/1676910, which needs to be merged first. This patch extends the fix to other callers.)

[Jeff Young (jeyjey)]

This is ready for review now that the parent merge went in.

[Jeff Young (jeyjey)]

Rebased patch (since the original was a bit stale).

[KiCad Janitor (kicad-janitor)]

Fixed in revision f69d499527137734d20f1c16ed4bdb8ac1cbb11d
https://git.launchpad.net/kicad/patch/?id=f69d499527137734d20f1c16ed4bdb8ac1cbb11d

[Wayne Stambaugh (stambaughw)]

@Jeff, I merged your patch but there is still a minor problem. I'm not sure if one of your previous patches or something else caused the change. When there is a failure loading a footprint library, none of the correctly loaded libraries are shown when selecting "List All" from the "Load Footprint" dialog. The previous behavior would show everything that was loaded which is a better option than not showing any footprints at all. I'm not sure when exactly this behavior changed. Would you please take a look at this when you get a chance? It's easy enough to test, just delete a footprint library that is in the footprint library table and it will fail. I can file a new bug report on this if you want me to.

[Jeff Young (jeyjey)]

Actually Dick added that back in 2013. But I agree that it’s a bad idea and I’ll fix it.

> On 14 Feb 2018, at 18:13, Wayne Stambaugh <email address hidden> wrote:
>
> @Jeff, I merged your patch but there is still a minor problem. I'm not
> sure if one of your previous patches or something else caused the
> change. When there is a failure loading a footprint library, none of
> the correctly loaded libraries are shown when selecting "List All" from
> the "Load Footprint" dialog. The previous behavior would show
> everything that was loaded which is a better option than not showing any
> footprints at all. I'm not sure when exactly this behavior changed.
> Would you please take a look at this when you get a chance? It's easy
> enough to test, just delete a footprint library that is in the footprint
> library table and it will fail. I can file a new bug report on this if
> you want me to.
>
> --
> You received this bug notification because you are a member of KiCad Bug
> Squad, which is subscribed to KiCad.
> https://bugs.launchpad.net/bugs/1674053
>
> Title:
> curl hangs loading footprints from github
>
> Status in KiCad:
> Fix Committed
>
> Bug description:
> Occasionally, when loading footprints from github, libcurl freezes in
> curl_easy_perform() waiting on data from the network (particularly on
> Win10 for me). Because there is no timeout, there is no way to
> interrupt it other than by force-quitting kicad.
>
> Please don't immediately run and fix this - as part of my work to add
> footprint selection to eeschema, I've reworked the footprint loading
> into an asynchronous loader, and that changes the ways we can handle
> this. I just want to discuss possibilities:
>
> 1. Add a fixed timeout. This is easily done with curl_easy_setopt();
> we'd have to decide what the timeout should be. Downside is that it's
> a bit ugly. I'm sure no matter what timeout we pick, someone will
> insist it's too short for their slow connection.
>
> 2. Add a configurable timeout. Same as #1 but we also need to make the
> timeout configurable - a bit difficult as the footprint loader
> currently has no simple way I can find to get settings to it, and
> because the timeout setting is not specific to one application (we
> currently don't have a "common" settings panel/category).
>
> 3. Add an abort function. With the asynchronous loader this is
> possible now. libcurl can have a progress callback, which is allowed
> to return a value to abort the transfer. This would require a change
> to the plugin API, because the command to abort has to come all the
> way from whatever called FP_LIB_TABLE::FootprintLoad. Implementation
> would be simple though: async loader objects holds an atomic_bool
> "m_abort_transfer"; progress callback checks it; async loader Abort()
> function sets m_abort_transfer true and joins the threads.
>
> Opinions? I'm in favor of #3 but it's also the most work.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/kicad/+bug/1674053/+subscriptions

[Jeff Young (jeyjey)]

I take that back: it pre-dates even Dick's addition of multi-threading in 2013.

Separate issue logged: https://bugs.launchpad.net/kicad/+bug/1749572