sandia-minimega / minimega

minimega
GNU General Public License v3.0
148 stars 67 forks source link

Prevent Locking when Downloading File in miniweb #1475

Closed jacdavi closed 1 year ago

jacdavi commented 1 year ago

Previously, downloading a file on miniweb would lock minimega and the UI for the duration of the download. If the download was cancelled, things would remain locked for an extended period of time.

This PR makes miniweb create a new connection to minimega for each file download. This allows miniweb to still send other commands through its normal channel while handling a file. This also fixes the issue of a cancelled download as the new connection will simply be closed. You can also now download multiple files at the same time.

activeshadow commented 1 year ago

@aherna @jacdavi just FYI, there will be a conflict between this PR and my PR #1474, since my PR removes the use of cmdLock completely in lieu of a channel to serialize commands.

jacdavi commented 1 year ago

@activeshadow Yeah I was aware that I'll probably need to rewrite the changes, but wanted to get the PR posted.

For your PR, I was curious how it works with the read api since I used the same pattern here. I'm not super familiar with Go yet, so it wasn't clear to me how you avoided blocking on that call

activeshadow commented 1 year ago

@jacdavi by using a channel instead of a mutex, commands added to the channel get processed in a FIFO fashion. This means different commands from different goroutines (read API, miniweb requests, etc) can be intertwined in the cmd channel without blocking or having to worry about locking. It's the whole "share memory by communicating" philosophy. Hopefully it ends up being a fully working solution... we'll see how your testing goes in #1474. I've been able to replace mutexes with channels like this many times in the past with success so I'm confident it'll work here too.

aherna commented 1 year ago

@jacdavi Feel free to resolve conflict

jacdavi commented 1 year ago

@aherna ok, should be all set. Thanks to @activeshadow only the miniweb side required changes now.

aherna commented 1 year ago

@jacdavi this all set so merge in?

jacdavi commented 1 year ago

yep!