[WIP] Move URL loading operations off the main thread

[verbosus]

Mechanic2 right now feels a lot slower than it could because it makes all its remote network calls as synchronous operations (via urllib.request.urlopen) which blocks the main thread. This results in a spinning beachball for the user.

I spent some time coming up with a replacement URL fetching mechanism which is a very thin wrapper around the system’s URLSession. This exposes another issue with the current Mechanic2, which is that extensionstore.robofont.com is served via http instead of https. URLSession errors out, saying that the App Transport Security policy requirements don’t allow non-https connections.

This is easily fixed, actually: the server hosting extensionstore.robofont.com does accept requests on port 443 and does have an SSL certificate, but it is expired. Renew that one (perhaps via https://letsencrypt.org?), change the default URL for the extensionstore to an https one, and… presto!

A couple of issues this PR introduces is that images now a. don’t get cached as before and b. load asynchronously, which causes a little bit of scrolling jerkiness when they do load. These two are minor, but noticeable. I haven’t had the time to come up with a fix for these two just yet.

WARNING: This PR is not quite ready to be merged into trunk! But it’s enough to show a proof of concept of how things might work. I figured there’s enough stuff in it already I might as well send it out to get feedback.

[LettError]

I downloaded the https://github.com/verbosus/mechanic-2 master branch, installed. First time it worked, showed the list of extensions. Second and subsequent attempts end in a stalled "Loading extensions" sheet in the Mechanic window. Output has a traceback. RF continues to work, but needs a force quit to get rid of the Mechanic window.

​  File "PyObjCTools/AppHelper.pyc", line 79, in scheduleCallWithDelay_
  File "PyObjCTools/AppHelper.pyc", line 97, in performCall
  File "/Users/erik/Library/Application Support/RoboFont/plugins/Mechanic2.roboFontExt/lib/mechanic2/ui/controller.py", line 152, in _makeExtensionRepositories
    for extensionData in _data:
TypeError: 'NoneType' object is not iterable

With Version 3.3b (build 1909120913)

[verbosus]

Thanks for checking this out Erik! You need to make a small adjustment to your extension URL settings to make my version fully work. Open Mechanic’s settings window, you’ll see two URLs: one of them is http the other https. Set both to https and you should be fine.

[verbosus]

(Let me catch that None in the meantime so at least the Mechanic window doesn’t get stuck.)

[LettError]

Thanks, that works. I don't.. have the extensionstore items at the top of the list anymore. Do the urls in the json also need to be https?

For instance: https://extensionstore.robofont.com/data.json contains "link" : "http://extensionstore.robofont.com/extensions/prepolator/",

[verbosus]

Eventually if the PR is accepted they should be changed to https, too. Right now the PR is forcing them to execute over https on its own.

[verbosus]

Cool! I looked at threaded url readers a while ago to process data in the back, but the main issue back then was to port it back to a UI, which is always in the main thread.

Yes, that’s various ways to go about it. One of them is performSelectorOnMainThread, which . PyObjC also has a convenience method for: callAfter, which also executes on the main thread.

Ive already updated https://extensionstore.robofont.com/data.json with a valid certificate.

Awesome!

[verbosus]

With 34dc772a9c0182601e65c6855591accda9f7c18f all connections always go through https, which should provide a safe upgrade path for people on the old version of Mechanic2.

[verbosus]

Also included a fix for issue #17

[gferreira]

this is great, super fast.

I noticed that local streams stopped working / are not allowed* anymore. for example:

file:///myFolder/customExtensionStream.json

could this function be restored? it’s super useful for testing extensions before publishing.

thanks!

* I get this error when the URL validation fails:

Traceback (most recent call last):
  File "PyObjCTools/AppHelper.pyc", line 79, in scheduleCallWithDelay_
  File "PyObjCTools/AppHelper.pyc", line 97, in performCall
  File "/Users/gferreira/Library/Application Support/RoboFont/plugins/Mechanic2.roboFontExt/lib/mechanic2/ui/settings.py", line 79, in _checkURLCallback
    logger.error(e)
UnboundLocalError: local variable 'e' referenced before assignment

[verbosus]

Thanks for the feedback. Let me look into the local streams.

[verbosus]

@gferreira Gustavo, I had a bogus logging statement which might have interfered with individual extensions, even though I don’t think I touched that specific part of the code. See if things work for you now. By the way, where can I find a simple .mechanic file to install? Thanks!

[gferreira]

hi @verbosus, thanks for the update.

single extension items seem to be working fine – here’s an example .mechanic file.

I was referring to local streams (top list):

I think it breaks because of URLReader(force_https=True) (?)

here is an actual extension stream file for testing: roboDocsExtensionStream.json

thanks for looking into it!

[verbosus]

Thanks again, @gferreira! I believe I fixed this.

[verbosus]

I managed to avoid the source of the scrolling jerkiness that I was mentioning in the original post and added a small cache for the icons. I’m tracking down a small issue with the first few icons not displaying right away even after having loaded.

[verbosus]

OK, I managed to track down the issue with icons not displaying after having loaded. Because of the async loading and wanting to avoid scrolling jerkiness (which would happen if we just called setNeedsDisplay on the main NSTableView whenever a new icon was available), the solution is a little more involved than I’d like but it does seem to work.

[verbosus]

@typemytype I squashed all the commits together for easier reviewing.

[verbosus]

Sorry, I couldn’t resist tweaking this a tiny bit more: I added a bit of code to prioritize fetching and displaying the icons in the visible area of the NSTableView, so the placeholder is shown for the absolute minimum time it needs to. Meanwhile, the other icons are also loaded and cached so users should never see the placeholders afterwards. I also squashed this commit so everything is neat for review.

[verbosus]

I’m not sure this is necessary anymore after this change.

[gferreira]

I’ve been using Antonio’s version for a week now – it works great. the performance improvement is huge, user experience is much much better.

@verbosus t h a n k s !

[typemytype]

that dialog can be removed

I would keep the same pace: once a day to check for updates (as github does not allows super many requests) based on the mechanic pref.

and popup Mechanic when all extensions are checked, if this is possible as you dont know when a all requests are done. If not possible it could popup Mechanic when the first needs-update-extension is found.

[typemytype]

Sorting extension on extensionNeedsUpdate seems not to work

[verbosus]

@typemytype try now, sorting and updating should be fixed now. I’m having some trouble with defconAppKit.windows.baseWindow.BaseWindowController.startProgress(): because of the asyncronous way the code currently works, I need to start a progressWindow in one method, store its value in a property and retreive it later from another method so I can update it and eventually close it. I’m calling self.startProgress("message") as other parts of the code did before. But sometimes the progressWindow I have already started is no either no longer there, or it seems to reference a window object which is None.

[typemytype]

a trick for a progressbar could be:

• start progress with a length of total amount of extensions, dont close it
• start each extension check for update
• when called back in the main thread increment the progressbar
• when to total of increments is equal to the total amount of extensions --> close it

[typemytype]

got this:

extensionItem.py", line 381, in _checkForUpdatesCallback
    self._remoteVersion = info.get("version")
UnboundLocalError: local variable 'info' referenced before assignment

[verbosus]

That’s pretty much how it’s working right now. I get erratic crashes when the progress bars go away in-between calls, but can’t really reproduce. I suggest people start testing the current version for a bit & let’s see if we hit any crashes (it crashes Mechanic, not the whole Robofont app.)

[typemytype]

maybe a simple spinner next to the button is enough, a un-closable sheet is annoying

[typemytype]

and sorting works!!!

[typemytype]

how long does the URLReader cache a request?

[verbosus]

Let me correct myself: right now URLReader uses the default URLSession caching. Try it out: open Mechanic and let it load the extensions off the remote data streams. Then switch your internet connection off, close and reopen Mechanic. The extensions will be coming from the URLSession cache.

I had to remove a few calls to your remember memoization decorator because with the async loading it was caching the wrong values. I treated icons separately because they were a bit expensive: I added a small in-memory cache for these so they don’t get reloaded unless one restarts the whole app. Now that I write this, I’m thinking it’d be even better to persist them on-disk so they’d stick around across app restarts.

When checking for updates we want to be careful with caching or we’d get stale updates. We could conceivably play with a push mechanism for this rather than the current polling one, but for right now it seems overkill.

It’s possible to configure URLSession to do some more fine-grained cache in memory or on disk and I do plan to experiment with that once the current kinda-caching version is settled.

[arialcrime]

First of all thanks a lot for all your efforts to improve the overall performance and experience of Mechanic, this is very exciting!

Unfortunately, after installing Antonio’s current version, I am experiencing a similar problem to what Erik described above. I am stuck on the “Loading extensions…” sheet and can’t access Mechanic’s preferences etc.

The output looks like this:

Traceback (most recent call last):
  File "main.py", line 4, in <module>
  File "lib/scripting/nameSpaceCallbacks.pyc", line 51, in OpenWindow
  File "/Users/b.bramboeck/Library/Application Support/RoboFont/plugins/Mechanic2.roboFontExt/lib/mechanic2/ui/controller.py", line 141, in __init__
  File "/Users/b.bramboeck/Library/Application Support/RoboFont/plugins/Mechanic2.roboFontExt/lib/mechanic2/ui/controller.py", line 261, in loadExtensions
  File "/Users/b.bramboeck/Library/Application Support/RoboFont/plugins/Mechanic2.roboFontExt/lib/urlreader.py", line 31, in __init__
  File "/Users/b.bramboeck/Library/Application Support/RoboFont/plugins/Mechanic2.roboFontExt/lib/urlreader.py", line 71, in init
AttributeError: '__NSCFURLSessionConfiguration' object has no attribute 'setWaitsForConnectivity_'

I am running Version 3.3b (build 1910091627) on macOS 10.12.6

Thanks in advance for the help!

[verbosus]

Thanks for testing, @arialcrime! The waitsForConnectivity property on NSURLSessionConfiguration has been added in macOS 10.13, so it makes sense that it would not be available on your 10.12.6 machine. I just pushed a new version that checks that the property is available before trying to set it, so this should get you unstuck. Give it a shot.

This new version also adds a cleaned-up URLReader, which I’ve also split up in its own repository and added some examples and unit tests to. With this version of URLReader, Mechanic now caches the extension icons on-disk so that they’re available offline, fixing #20.

I am still working on a change to split the core of Mechanic off into its own testable module, but that’s a bigger change for another time.

[arialcrime]

Yes, that works. I get the list, can “Check for Updates”, and also see the result now. But when I select an extension (eg. ShapeTool) and click “Install” it crashes Robofont. Sadly, there’s nothing in the Output window or the log. Any pointers where I could find some useful info?

[verbosus]

If it’s a hard crash, you should see a crash log in Console.app. Let me know what you find!

[verbosus]

Alright folks, thanks to some very generous testing by @arialcrime (grande Benedikt!) I was able to track down what the issue with his crash was. While looking into this I made URLReader somewhat more robust against bogus input and squashed another few bugs in the Mechanic UI—before the latest changes, the little badge next to an extension was not appearing right away after an install, now it works as it should.

[verbosus]

I merged a slight color tweak from @andyclymer (thanks, Andy!), increased the font weight ever so slightly and re-sync’d urlreader to the latest version.

[typemytype]

maybe we have to wrap this PR up, otherwise it will become an endless one.

The UI changes should be handled and discussed in a separate PR to keep oversight.

The main goals for this PR are:

• [x] loading speed
• [x] dont block the main UI while checking, so it can happen in the background during startup
• [x] less clicks to update as checking already happed in the background

@verbosus please add to this list if there are items missing

[andyclymer]

Ah yeah, I was trying to sneak a small color adjustment into @verbosus ' excellent work, let's save it for a separate discussion :)

[verbosus]

OK, I reverted the little UI changes & squashed the commits together. Ready for a final review, @typemytype!

[gferreira]

hello @verbosus,

I’m now getting an error when trying to add a new stream (using the latest version from your repository):

Traceback (most recent call last):
​  File "/Applications/RoboFont.app/Contents/Resources/lib/python3.7/vanilla/vanillaBase.py", line 499, in action_
    self.callback(sender)
  File "/Users/gferreira/Library/Application Support/RoboFont/plugins/Mechanic2.roboFontExt/lib/mechanic2/ui/settings.py", line 109, in addCallback
    urlreader = DefaultURLReader(force_https=True)
TypeError: 'URLReader' object is not callable
Traceback (most recent call last):
  File "lib/doodleDelegate.pyc", line 95, in sendEvent_
  File "/Applications/RoboFont.app/Contents/Resources/lib/python3.7/vanilla/vanillaBase.py", line 499, in action_
    self.callback(sender)
  File "/Users/gferreira/Library/Application Support/RoboFont/plugins/Mechanic2.roboFontExt/lib/mechanic2/ui/settings.py", line 109, in addCallback
    urlreader = DefaultURLReader(force_https=True)
TypeError: 'URLReader' object is not callable

this was working before… I think one of the last commits must have broken it.

thanks in advance for looking into it!

[verbosus]

Hey @gferreira, thanks for testing, I believe I fixed this. All I can say is that I wish this code had tests (I’m still working on that in a private branch, blah blah blah…)

[gferreira]

@verbosus problem with adding streams fixed by your last commit, thanks.

one more issue: I have a valid private extension which is failing to install with your version of Mechanic, but can be installed with the current master version. the error messages say “Could not download the extension zip file” and “the request times out”. I’m sending you the .mechanic file off-list for testing.

[verbosus]

There’s a partially-quoted URL in your .mechanic file. Let me see if I can fix it from the URLReader side.

[typemytype]

@verbosus I would like to merge this! as this is already a fantastic update! Is there anything you think of that could hold me back from pushing the merge button?

[verbosus]

@typemytype ship it! 🤞

[LettError]

Yay!

[typemytype]

hurray!