cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Javier Puerto <jpue...@gmail.com>
Subject Re: File-transfer: delete target file on process error
Date Wed, 18 Jun 2014 07:48:25 GMT
I've explained my use case in the issue CB-6928 but as summary I'm creating
a simple rsync to keep the data in the device up to date.

2014-06-18 4:08 GMT+02:00 Ian Clelland <iclelland@chromium.org>:

> 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?
>

That's right. I use the file-transfer plugin because I see that it allows
custom headers so I thought that this way I can avoid an unnecessary HEAD
request first, just set the header and expect that the content is updated
if it's necessary or skipped in case that it's not.


>
> 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'm agree. The patch I sent just skips the buffer copy if a 304 is detected
as response status. The problem is that if we are using a temporary file as
target, the plugin will return an invalid file entry. Adding an extra
parameter to communicate a cached response should do the job but for me it
looks too complicated, error has an status parameter in the callback and
conceptually the file-transfer didn't transferred anything so use the error
callback it's fine with me defining a new status for caching.


>
> 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.'ve'
>

The developer should be responsible to set the headers that he wants. For
my use case, the "rsync" updates the device and the application will just
consume the resources so no more modifications in FS.


>
> 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.
>
>
I don't think that my use case is complicated, I just expect that the
file-transfer update the file if it's necessary. Definitively the current
behaviour is wrong as doesn't makes sense to read a 304 response, it's
empty. Try to avoid an initial HEAD request it's just to save an extra
request that it's not necessary. Using different request means that in the
worst case, I will have to do two request per resource to update.


> 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