Opened 12 years ago

Closed 8 years ago

Last modified 8 years ago

#394 closed defect (fixed)

cancel a download if space is very tight

Reported by: tomeu Owned by: humitos
Priority: Urgent Milestone:
Component: Browse Version: Git as of bugdate
Severity: Critical Keywords: r+, olpc-test-pending
Cc: manuq, humitos, erikos, godiard Distribution/OS: Unspecified
Bug Status: Assigned

Description

From (#172): For example stop before there's only 1MB free.

Attachments (8)

0001-Cancel-a-download-if-space-is-very-tight-SL-394.patch (2.3 KB) - added by humitos 9 years ago.
0001-Cancel-a-download-if-space-is-very-tight-SL-394.2.patch (2.7 KB) - added by humitos 9 years ago.
v2 - improved
download_file_with_webkit.py (1.6 KB) - added by humitos 9 years ago.
Example
0001-Cancel-a-download-if-space-is-very-tight-SL-394.3.patch (4.7 KB) - added by humitos 9 years ago.
This patch needs a patched version of sugar that implements "enough_space" method in Activity class
sugar-toolkit-gtk3-1-2-Activity-class-method-to-check-enough-space.patch (1.7 KB) - added by humitos 9 years ago.
enough_space method on Activity class
sugar-toolkit-gtk3-2-2-Signal-to-check-free-available-space.patch (2.3 KB) - added by humitos 9 years ago.
error (LOW_SPACE) signal in datastore
0001-Cancel-a-download-if-space-is-very-tight-SL-394.4.patch (6.3 KB) - added by humitos 8 years ago.
This patch puts the "enough_space" in the Download class inside the Browse Activity, so we don't have to modify sugar3.activity.Activity class in this cycle
0001-Correct-amount-in-free-space-error-message-SL-394.patch (2.2 KB) - added by humitos 8 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 12 years ago by erikos

The problem I see here, is that we would need some sort of notification that the download has been stopped - or been canceled due to space constraints. And as we are in string freeze...

comment:2 Changed 12 years ago by erikos

  • Bug Status changed from New to Assigned
  • Milestone changed from 0.84 to 0.86

I think this needs proper handling with a dialog (string based).

comment:3 follow-up: Changed 11 years ago by timClicks

Would it be possible to alter Browse's behaviour before making a download? This could be achieved through an HTTP HEAD request, then checking for available space.

comment:4 Changed 11 years ago by bernie

  • Priority changed from Normal to Urgent

This bug occurs often in the field. Children download huge files, often in parallel, without thinking of the consequences. It's a very good way to screw up your datastore and make your system unbootable.

Thus, rising priority.

comment:5 in reply to: ↑ 3 Changed 11 years ago by bernie

  • Cc bernie added
  • Milestone changed from 0.86 to 0.90

Replying to timClicks:

Would it be possible to alter Browse's behaviour before making a download? This could be achieved through an HTTP HEAD request, then checking for available space.

Nope, because users download several things in parallel so space runs out in unpredictable ways. We should figure out a way to interrupt downloads before the filesystem becomes full.

comment:6 Changed 9 years ago by manuq

  • Milestone changed from 0.90 to 0.98

comment:7 Changed 9 years ago by humitos

  • Cc godiard humitos added

I tested it in build 18 and it is still happening.

I think this bug is important because it makes Sugar to not work anymore. I mean, there is no way to remove the file because a dialogue with the message "The Journal is full" with the button "Show Journal" is popped up all the time and it doesn't allow us to do anything.

I had to login with Ctrl + Alt + F2 and remove a video that I was downloading to make Sugar to work again.

How we should implement this? Should we check the free space on every "chunk" downloaded by Browse?

Changed 9 years ago by humitos

v2 - improved

comment:9 Changed 9 years ago by greenfeld

I am concerned that 50 MB might be too much space to save. A 2 GB XO-1.5 with an unedited OLPC build will only have about 170 MB free to start with.

A bootable Fedora SoaS USB with an overlay partition also might have limited space available.

Refusing to download with ~30% provided space available seems like a bit much in this case. Perhaps 10 MB or 20 MB would be better.

But then this might just be bikesheding and not relevant except to the few XO deployments that actually purchased 2 GB units.

comment:10 Changed 9 years ago by humitos

  • Owner changed from erikos to humitos
  • Status changed from new to accepted

Changed 9 years ago by humitos

Example

comment:11 Changed 9 years ago by humitos

  • Keywords patch added

Changed 9 years ago by humitos

This patch needs a patched version of sugar that implements "enough_space" method in Activity class

Changed 9 years ago by humitos

enough_space method on Activity class

Changed 9 years ago by humitos

error (LOW_SPACE) signal in datastore

comment:12 Changed 9 years ago by humitos

  • Cc erikos added
  • Keywords r? added; patch removed

Simon, I've just uploaded the sugar-toolkit-gtk3 patches related to this one.

Changed 8 years ago by humitos

This patch puts the "enough_space" in the Download class inside the Browse Activity, so we don't have to modify sugar3.activity.Activity class in this cycle

comment:13 Changed 8 years ago by manuq

  • Keywords r+ olpc-test-pending added; r? removed
  • Resolution set to fixed
  • Status changed from accepted to closed

Finally pushed f96848a9 . Thanks Manuel.

comment:14 follow-up: Changed 8 years ago by greenfeld

  • Keywords olpc-test-pending removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

The patch seems to work in Browse-148, but has an order of operations issue in the error dialog.

The section

free_space_mb = self._free_available_space( 
    path=self.temp_path) - SPACE_THRESHOLD \ 
    / 1024.0 ** 2 

likely should read

free_space_mb = (self._free_available_space( 
    path=self.temp_path) - SPACE_THRESHOLD) \ 
    / 1024.0 ** 2 

comment:15 in reply to: ↑ 14 Changed 8 years ago by godiard

  • Cc manuq added; bernie godiard removed
  • Keywords r+ removed

Humitos, manuq, can you see at the last comment from Sam?

A comment in _free_available_space explaining what unit is used (apparently return bytes) should be good. Also enough_space method say the size parameter should be in kb,
but is not consistent with the use of _free_available_space inside it.

comment:16 Changed 8 years ago by godiard

  • Cc godiard added

comment:17 Changed 8 years ago by humitos

  • Keywords r? added

I attached a new patch to fix what Sam mentioned in the previous comment and this patch adds as well some comments about the units used in the code.

comment:18 Changed 8 years ago by manuq

  • Keywords r+ olpc-test-pending added; r? removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Good catch. Pushed humitos patch as f40101a4 .

comment:19 Changed 8 years ago by dnarvaez

  • Milestone 0.98 deleted

Milestone 0.98 deleted

Note: See TracTickets for help on using tickets.