Introduce download-store & 2 steps download #256

aenario · 2017-01-27T14:49:19Z

No description provided.

codecov-io · 2017-01-27T15:25:54Z

Codecov Report

Merging #256 into master will increase coverage by 0.04%.

@@            Coverage Diff             @@
##           master     #256      +/-   ##
==========================================
+ Coverage   74.21%   74.26%   +0.04%     
==========================================
  Files          69       70       +1     
  Lines        5345     5468     +123     
==========================================
+ Hits         3967     4061      +94     
- Misses        934      952      +18     
- Partials      444      455      +11

Impacted Files	Coverage Δ
pkg/vfs/archive.go	`80.32% <100%> (+2.96%)`	✅
web/files/files.go	`74.66% <70.21%> (-0.95%)`	❌
pkg/vfs/download_store.go	`79.16% <79.16%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6abe031...2689490. Read the comment docs.

nono · 2017-01-27T15:39:28Z

docs/files.md

@@ -592,7 +592,7 @@ Location: http://cozy.example.com/files/9152d568-7e7c-11e6-a377-37cbfb190b4b

 ### POST /files/archive

-Create an archive and download it. The body of the request lists the files and
+Create an archive. The body of the request lists the files and


Would it be difficult to have content negotiation on that endpoint? When the client asks for application/vnd.api+json (JSON-API), the stack gives it a link for downloading the archive. But if it asks anything else, the stack sends the archive directly. I think it can be easier for apps that are not in a browser to have the possibility to download the archive in one request and not two.

nono · 2017-01-27T15:41:10Z

docs/files.md

+}
+```
+
+### GET /files/archive/:key/:name?


Why the ?

nono · 2017-01-27T15:45:14Z

pkg/vfs/download_store.go

+}
+
+func (s *memStore) AddFile(f string) (string, error) {
+	fref := &fileRef{


Maybe before adding a file, we should remove all the filesRef that have expired in s.Files to avoid memory leak

nono · 2017-01-27T15:45:26Z

pkg/vfs/download_store.go

+	return key, nil
+}
+
+func (s *memStore) AddArchive(a *Archive) (string, error) {


And the same for archive

nono · 2017-01-30T10:48:33Z

docs/files.md

@@ -62,7 +62,7 @@ Location: http://cozy.example.com/files/6494e0ac-dfcb-11e5-88c1-472e84a9cbee
    "type": "io.cozy.files",
    "id": "6494e0ac-dfcb-11e5-88c1-472e84a9cbee",
    "meta": {
-      "rev": "1-ff3beeb456eb"
+      "rev": "1-ff3beeb456eb",


In JSON, the trailing comma is not allowed...

nono · 2017-01-30T10:49:16Z

docs/files.md

-        },
-        "data": [
-          { "type": "io.cozy.playlists", "id": "94375086-e2e2-11e6-81b9-5bc0b9dd4aa4" }
-        ]


Why did you remove the example of referenced_by?

bad merge I suppose. fixing now

nono · 2017-01-30T10:49:35Z

docs/files.md

@@ -317,9 +309,6 @@ Location: http://cozy.example.com/files/9152d568-7e7c-11e6-a377-37cbfb190b4b
 **Note**: for an image, the links section will also include a link called
 `thumbnail` to the thumbnail URL of the image.

-**Note**: see [references of documents in VFS](references-docs-in-vfs.md) for
-more informations about the references field.
-


Why did you remove this note?

nono · 2017-01-30T13:02:24Z

docs/files.md

+```http
+GET /files/archive/4521DC87 HTTP/1.1
+Accept: application/zip
+Content-Length: 12345


There is a + to remove

jinroh · 2017-01-30T16:03:51Z

pkg/vfs/download_store.go

+const downloadStoreTTL = 1 * time.Hour
+const downloadStoreCleanInterval = 1 * time.Hour
+
+var storeStore map[string]*memStore


maps are not thread safe, you should have a mutex around this one since it is used concurrently

jinroh · 2017-01-30T16:05:18Z

pkg/vfs/download_store.go

+type memStore struct {
+	Archives map[string]*Archive
+	Files    map[string]*fileRef
+}


this memStore should also have at least one mutex to protect read/writes from Archives and Files fields

jinroh · 2017-01-31T10:08:09Z

pkg/vfs/download_store.go

+}
+
+func (s *memStore) GetFile(k string) (string, error) {
+	s.Mutex.Lock()


use defer s.Mutex.Unlock() to avoid a deadlock on error branches where the lock is not unlocked.

jinroh · 2017-01-31T10:08:27Z

pkg/vfs/download_store.go

+}
+
+func (s *memStore) GetArchive(k string) (*Archive, error) {
+	s.Mutex.Lock()


jinroh · 2017-01-31T10:11:24Z

pkg/vfs/download_store.go

+}
+
+func cleanDownloadStore() {
+	storeStoreMutex.Lock()


would use a defer Unlock() on all locking methods (even the correct ones), for clarity

jinroh · 2017-02-01T08:22:37Z

pkg/vfs/download_store.go

+}
+
+type memStore struct {
+	Mutex    *sync.Mutex


sync.Mutex are meant to be used as non-pointers that way you do not need to initialize them when creating a var mutex or struct containing one.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce download-store & 2 steps download #256

Introduce download-store & 2 steps download #256

aenario commented Jan 27, 2017

codecov-io commented Jan 27, 2017 •

edited

nono Jan 27, 2017

nono Jan 27, 2017

nono Jan 27, 2017

nono Jan 27, 2017

nono Jan 30, 2017

nono Jan 30, 2017

aenario Jan 30, 2017

nono Jan 30, 2017

nono Jan 30, 2017

jinroh Jan 30, 2017

jinroh Jan 30, 2017

jinroh Jan 31, 2017 •

edited

jinroh Jan 31, 2017

jinroh Jan 31, 2017

jinroh Feb 1, 2017

jinroh Feb 1, 2017

aenario Feb 2, 2017

Introduce download-store & 2 steps download #256

Introduce download-store & 2 steps download #256

Conversation

aenario commented Jan 27, 2017

codecov-io commented Jan 27, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinroh Jan 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 27, 2017 •

edited

jinroh Jan 31, 2017 •

edited