cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ian Clelland <iclell...@chromium.org>
Subject Re: File-transfer: delete target file on process error
Date Wed, 18 Jun 2014 02:08:26 GMT
On Tue, Jun 17, 2014 at 9:34 PM, Andrew Grieve <agrieve@chromium.org> wrote:

> If it's cached... won't it exist?
>

Exactly this. A 304 request should only be received in response to a
conditional GET request. There's generally no reason to send a conditional
GET unless you already have a cached copy of the file -- that is, unless
you really don't care about its contents; only whether it's changed. But if
that's the case, then why are you using the file-transfer plugin?

File-transfer is designed to be really simple. I think that we should allow
the developer to set If-Modified-Since headers any way that they like -- or
even If-None-Match, or crazier headers like If-Range, but we shouldn't set
those headers by default. Then, if a 304 is returned, then the right thing
to do is return *success* and not touch the file on disk at all.
File-transfer has fulfilled its API: the file exists where it should.

I don't think that we should automatically set the If-Modified-Since
header. My concern is that lots of other file operations could change that
date -- usually by modifying the file and setting it to *now* -- and in
that case, you might never get a newer copy of the resource you've
requested.

We could certainly add a flag to insert the header, based on the file's
timestamp. Then a dev who knows the file hasn't been touched can set it.

I think that anyone looking for something more complicated than this should
just use XHR and File, and manage the request and response headers
themselves.

Ian


>
> On Tue, Jun 17, 2014 at 11:56 AM, Javier Puerto <jpuerto@gmail.com> wrote:
>
> > I think it's better to use the error callback because for cached
> resources
> > doesn't makes sense to use the "Entry" as parameter as the target will
> not
> > exists..There's no error but the file transfer was unable to download
> > anything due to the 304 response so IMO the error callback could do the
> > job.
> >
> >
> > 2014-06-17 16:27 GMT+02:00 Andrew Grieve <agrieve@chromium.org>:
> >
> > > How about adding a second parameter to the callback? Android and iOS
> > > bridges both support this natively, and you can simulate it on other
> > > platforms by manually unpacking the parameters in your own callback.
> > >
> > >
> > > On Tue, Jun 17, 2014 at 4:18 AM, Javier Puerto <jpuerto@gmail.com>
> > wrote:
> > >
> > > > 2014-06-16 17:01 GMT+02:00 Andrew Grieve <agrieve@chromium.org>:
> > > >
> > > > > I think this behaviour has been around for a while, and makes sense
> > in
> > > > the
> > > > > majority of cases.
> > > >
> > > >
> > > > Yep, I did a "git blame" and this fragment of code was there from the
> > > > beginning.
> > > >
> > > >
> > > > > Best practice is to download to a temporary location,
> > > > > and then upon success move the file to its final spot.
> > > > >
> > > >
> > > > Yes, this is what I will have to do at the end. The thing is that I
> > want
> > > to
> > > > avoid a double step process. Anyway I'm still thinking that a hard
> > coded
> > > > and undocumented behaviour is not a good practice neither, mostly
> > because
> > > > there's the tools for the developer to act as he needs with the
> success
> > > or
> > > > error callback. It's about flexibility.
> > > >
> > > >
> > > > >
> > > > > That said, I think it'd be fine to add an option for not delete on
> > > error.
> > > > >
> > > >
> > > > It seems like the file transfer download is oriented to use a
> temporary
> > > > directory so it's fine for me as is (keep it simple). I think that an
> > > > explanation in the plugin documentation should be great so everyone
> can
> > > > figure how to use the download.
> > > >
> > > > About the issue CB-6928, my patch it's not valid for me as I will use
> > the
> > > > temporary directory now and probably for everyone so I will have to
> > > cancel
> > > > the pull request. The problem is still there but I wonder how can we
> > fix
> > > it
> > > > if the success callback argument is an "Entry" object, the ideal
> should
> > > be
> > > > to be able to communicate the caching status. I will change the patch
> > to
> > > > use the error callback to communicate the caching status, it's not an
> > > error
> > > > but I don't see other way. I will add also the documentation for the
> > new
> > > > caching status code and open a new pull request.
> > > >
> > > >
> > > > >
> > > > >
> > > > > On Mon, Jun 16, 2014 at 5:39 AM, Javier Puerto <javier@apache.org>
> > > > wrote:
> > > > >
> > > > > > Hi Cordova developers,
> > > > > >
> > > > > > I'm creating a system to download/update several resources from
a
> > > > server
> > > > > to
> > > > > > the device and I've observe a behaviour that breaks my use case.
> > > > > >
> > > > > > After fix the issue CB-6928, I'm able to download/update all
the
> > > > > resources
> > > > > > without problems. My next test was to try to download the
> resources
> > > but
> > > > > > with no server response (stopped). I've found that the plugin
is
> > > > deleting
> > > > > > my target file silently because there's was an error.
> > > > > >
> > > > > > I think that the developer should be the responsible to delete
or
> > > leave
> > > > > the
> > > > > > "corrupted" file because the file-transfer plugin already
> > > communicates
> > > > > the
> > > > > > error, I don't think that the hardcoded behaviour is a good
> > solution.
> > > > >  What
> > > > > > do you think?
> > > > > > I can open a new issue and provide the patch and test case
> (Android
> > > > only)
> > > > > >
> > > > > > Best regards.
> > > > > >
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message