cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Grieve <agri...@chromium.org>
Subject Re: File-transfer: delete target file on process error
Date Wed, 18 Jun 2014 14:43:49 GMT
Okay, think I got it and sounds good to me.


On Wed, Jun 18, 2014 at 3:48 AM, Javier Puerto <jpuerto@gmail.com> wrote:

> 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