Implement FileStreamProxy that calls FileStream methods on FileThread for FileAPI

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

To support asynchronous file operations in FileAPI, we need a proxy module that proxies the requests and calls corresponding FileStream methods on the file thread.

[Kinuko Yasuda]

Created attachment 52755 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 52755 [details]
Patch

The patch depends on another patch for bug 37217.

[Kinuko Yasuda]

Created attachment 52761 [details]
Patch

[Jian Li]

Comment on attachment 52761 [details]
Patch

> --- /dev/null
> +++ b/WebCore/html/FileStreamProxy.cpp
> +#include "FileThread.h"
> +#include "FileThreadTask.h"
This include should be placed above so that it will be in the alphabetical
order.

> FileStreamProxy::~FileStreamProxy()
> {
>     if (fileThread())
>         fileThread()->unscheduleTasks(m_stream.get());
Is it possible that m_context is destructed before FileStreamProxy instance?

> +void FileStreamProxy::read(char* buffer, int length)
> +{
> +    fileThread()->postTask(createFileThreadTask(m_stream.get(), &FileStream::read, buffer, length));
> +}
> +
> +void FileStreamProxy::write(Blob* blob, long long position, size_t length)
We should use either size_t or int for length in read() and write(), for
consistency.

> +FileThread* FileStreamProxy::fileThread()
> +{
> +    ASSERT(m_context->fileThread()); // FIXME
What is this FIXME about? 
> +    return m_context->fileThread();
> +}

> +void FileStreamProxy::abort()
> +{
> +    if (fileThread())
Do we really need to check for fileThread()?
> +        fileThread()->unscheduleTasks(m_stream.get());
> +}

> +static void notifyWriteOnContext(ScriptExecutionContext* context, FileStreamProxy* stream, int nwritten)
Better to name it as bytesWritten.

[Kinuko Yasuda]

Created attachment 52817 [details]
Patch

[Kinuko Yasuda]

(In reply to comment #4)
> (From update of attachment 52761 [details])
> > --- /dev/null
> > +++ b/WebCore/html/FileStreamProxy.cpp
> > +#include "FileThread.h"
> > +#include "FileThreadTask.h"
> This include should be placed above so that it will be in the alphabetical
> order.

The order is enforced by check-webkit-style, so I assume the current order is ok.

> > FileStreamProxy::~FileStreamProxy()
> > {
> >     if (fileThread())
> >         fileThread()->unscheduleTasks(m_stream.get());
> Is it possible that m_context is destructed before FileStreamProxy instance?

I fixed some refs/destructor issues.  The new code assumes the object that holds a reference to FileStreamProxy calls FileStreamProxy::stop() in its destructor to safely shutdown the context/thread.

> > +void FileStreamProxy::write(Blob* blob, long long position, size_t length)
> We should use either size_t or int for length in read() and write(), for
> consistency.

Changed size_t to int.
 
> > +FileThread* FileStreamProxy::fileThread()
> > +{
> > +    ASSERT(m_context->fileThread()); // FIXME
> What is this FIXME about? 

If thread creation fails it will cause SEGV... but currently I have no idea to 'fix' it, so I removed the comment.

> > +static void notifyWriteOnContext(ScriptExecutionContext* context, FileStreamProxy* stream, int nwritten)
> Better to name it as bytesWritten.

Done.

[Kinuko Yasuda]

Created attachment 52883 [details]
Patch

[Kinuko Yasuda]

Made one more fix (added fileThread()->unscheduleTasks() in FileStream::stop()).

[Jian Li]

Comment on attachment 52883 [details]
Patch

> diff --git a/WebCore/html/FileStreamProxy.cpp b/WebCore/html/FileStreamProxy.cpp
> new file mode 100644
> index 0000000..59abd68
We should not have the above info in the patch. Otherwise, you will make svn think it
is branched from other existing file, like previously landed patch:
  http://trac.webkit.org/changeset/57229

> +void FileStreamProxy::abort()
> +{
> +    fileThread()->unscheduleTasks(m_stream.get());
> +}
> +
> +void FileStreamProxy::stop()
> +{
> +    fileThread()->unscheduleTasks(m_stream.get());
> +    fileThread()->postTask(createFileThreadTask(m_stream.get(), &FileStream::stop));
> +}
It seems to be quite confusing with both abort() and stop() and they seem to do the similar things.
For stop(), you call unscheduleTasks to remove all file thread tasks and then call postTask
to add a new one. This seems to be a bit unnatural. Can you illustrate how the whole destruction
logic work?

> +static void notifyGetSizeOnContext(ScriptExecutionContext* context, FileStreamProxy* proxy, long long size)
> +{
> +    ASSERT(context->isContextThread());
Probably this ASSERT is really not needed. I think it would be better to have ASSERTs on did*** methods to
ensure they're called from file thread; and ASSERTS on open/read/write/... methods to ensure they're called
from non-file thread.

> +void FileStreamProxy::didStop()
> +{
> +    m_context->postTask(createCallbackTask(&notifyStopOnContext, this));
> +}
> +
> +void FileStreamProxy::didStopAndDestroy()
didStop() is called on file thread and didStopAndDestroy is called on context thread and both of them
are prefixed with "did", which increases the difficulty to understand.

[Kinuko Yasuda]

(In reply to comment #9)
> (From update of attachment 52883 [details])
> > diff --git a/WebCore/html/FileStreamProxy.cpp b/WebCore/html/FileStreamProxy.cpp
> > new file mode 100644
> > index 0000000..59abd68
> We should not have the above info in the patch. Otherwise, you will make svn
> think it
> is branched from other existing file, like previously landed patch:
>   http://trac.webkit.org/changeset/57229

Hmm they're done by webkit-patch.  I'll look into how I can avoid that.
 
> > +void FileStreamProxy::stop()
> > +{
> > +    fileThread()->unscheduleTasks(m_stream.get());
> > +    fileThread()->postTask(createFileThreadTask(m_stream.get(), &FileStream::stop));
> > +}
> It seems to be quite confusing with both abort() and stop() and they seem to do
> the similar things.
> For stop(), you call unscheduleTasks to remove all file thread tasks and then
> call postTask
> to add a new one. This seems to be a bit unnatural. Can you illustrate how the
> whole destruction
> logic work?

Abort() is straightforward, it needs to stop any (pending) writes/reads, thus it unschedules tasks.  it's just an API and by definition it doesn't have anything to do with destructing FileStreamProxy/FileStream.

Stop() is for enforcing a destruction order and closing a file handle on the file thread.  It shouldn't be called for other purpose.
It might be written better, but the logic I thought is:

0. FileStreamProxy holds refs for ScriptExecutionContext and FileStream, so they must be available throughout the proxy's life cycle.
1. when we need to stop/destroy FileStreamProxy, we call FileStreamProxy::stop() and then deref the proxy.
    (e.g. m_streamProxy->stop(); m_streamProxy = 0;)
     FileStreamProxy holds its own ref so it won't be deleted until it really thinks it's done.
2. FileStreamProxy::stop() calls unscheduleTasks to remove any pending writes/reads (i.e. tasks still in the queue) for the stream.
3. FileStreamProxy::stop() posts a task to close the file and waits for the close (and any ongoing tasks) to be completed.
4. When FileStreamProxy::didStop() is called we can safely say that the file is closed and there're no ongoing/pending tasks that need call back to the proxy.
5. FileStreamProxy::didStopAndDestroy() derefs itself and the context.

> > +static void notifyGetSizeOnContext(ScriptExecutionContext* context, FileStreamProxy* proxy, long long size)
> > +{
> > +    ASSERT(context->isContextThread());
> Probably this ASSERT is really not needed. I think it would be better to have
> ASSERTs on did*** methods to
> ensure they're called from file thread; and ASSERTS on open/read/write/...
> methods to ensure they're called
> from non-file thread.

Sounds good.  notifyXXX are all static methods and they are less likely to be called unexpectedly.
> > +void FileStreamProxy::didStop()
> > +{
> > +    m_context->postTask(createCallbackTask(&notifyStopOnContext, this));
> > +}
> > +
> > +void FileStreamProxy::didStopAndDestroy()
> didStop() is called on file thread and didStopAndDestroy is called on context
> thread and both of them
> are prefixed with "did", which increases the difficulty to understand.

I've tried to name it better but haven't come up with a good idea.

[Kinuko Yasuda]

Created attachment 52921 [details]
Patch

[Kinuko Yasuda]

Made a minor cleanup.  Removed didStopAndDestroy method.

The patch still has ' new file mode...' part.  I'll try fixing it...
> new file mode 100644
> index 0000000..59abd68

[Jian Li]

Comment on attachment 52921 [details]
Patch

I think the way to fix "new file mode..." problem in the patch is to create a new file
from the scratch and then copy and paste the content over. You need to revert all new
files just added and then create files and add them to the repository.

> diff --git a/WebCore/html/FileStreamProxy.cpp b/WebCore/html/FileStreamProxy.cpp
> +FileStreamProxy::FileStreamProxy(ScriptExecutionContext* context, FileStreamClient* client)
> +    : m_context(context)
> +    , m_client(client)
> +    , m_stream(FileStream::create(this))
> +{
> +    ref();
Could you please add the comment about why we need to keep its own ref here?

> +void FileStreamProxy::abort()
> +{
> +    fileThread()->unscheduleTasks(m_stream.get());
> +}
> +
> +void FileStreamProxy::stop()
> +{
> +    fileThread()->unscheduleTasks(m_stream.get());
> +    fileThread()->postTask(createFileThreadTask(m_stream.get(), &FileStream::stop));
> +}
I still think we do not need both abort() and stop(). Since we are not going to resume
after abort, can we simply call stop() from FileReader/FieWriter::abort()?

> diff --git a/WebCore/html/FileStreamProxy.h b/WebCore/html/FileStreamProxy.h
> +class FileStreamProxy : public RefCounted<FileStreamProxy>, public FileStreamClient {
> +protected:
If we're not going to have other child classes derived from FileStreamProxy,
it would be better to make it "private".

[Kinuko Yasuda]

Created attachment 53006 [details]
Patch

[Kinuko Yasuda]

Created attachment 53009 [details]
Patch

[Kinuko Yasuda]

(In reply to comment #13)
> (From update of attachment 52921 [details])
> I think the way to fix "new file mode..." problem in the patch is to create a
> new file
> from the scratch and then copy and paste the content over. You need to revert
> all new
> files just added and then create files and add them to the repository.

I tried something similar to this.  Hope it'll work this time.

> > diff --git a/WebCore/html/FileStreamProxy.cpp b/WebCore/html/FileStreamProxy.cpp
> > +FileStreamProxy::FileStreamProxy(ScriptExecutionContext* context, FileStreamClient* client)
> > +    ref();
> Could you please add the comment about why we need to keep its own ref here?

Done.
 
> > +void FileStreamProxy::stop()
> > +{
> > +    fileThread()->unscheduleTasks(m_stream.get());
> > +    fileThread()->postTask(createFileThreadTask(m_stream.get(), &FileStream::stop));
> > +}
> I still think we do not need both abort() and stop(). Since we are not going to
> resume
> after abort, can we simply call stop() from FileReader/FieWriter::abort()?

OK, I removed the abort method and added some more comments (chose stop() over abort() just for consistency).

> > diff --git a/WebCore/html/FileStreamProxy.h b/WebCore/html/FileStreamProxy.h
> > +class FileStreamProxy : public RefCounted<FileStreamProxy>, public FileStreamClient {
> > +protected:
> If we're not going to have other child classes derived from FileStreamProxy,
> it would be better to make it "private".

Right, I've been missing to fix it from another merge... Done.

[Jian Li]

Comment on attachment 53009 [details]
Patch

Only some minor things to resolve.

> diff --git a/WebCore/html/FileStreamProxy.cpp b/WebCore/html/FileStreamProxy.cpp
> +static void derefProxyOnContext(ScriptExecutionContext*, FileStreamProxy* proxy)
> +{
> +    proxy->deref();
Can we add an ASSERT before this line, like:
       ASSERT(proxy->hasOneRef());
This way, we can ensure that the caller, i.e., FileReader/FileWriter, has already
released the reference and now it should only has ref count 1 now.

> diff --git a/WebCore/html/FileStreamProxy.h b/WebCore/html/FileStreamProxy.h
> +// A proxy module that calls corresponding FileStream methods on the file thread.  Note: you must call stop() before destroying a FileStreamProxy instance, e.g. in the desctructor of an object that holds a reference to the proxy.
Probably we could say the following for Note part:
   // ... Note: you must call stop() first and then release the reference to the FileStreamProxy instance.

> +    // Stop() stops any pending tasks, close the file and derefs itself; the caller should not recycle the instance after calling stop().
It is not needed to repeat stop() in the comment. Also we should use "closes" to be consistent with "stops".
Probably it is better to say "destruct".
      // Stops the proxy and schedules it to be destructed. All the pending tasks will be aborted and the file stream will be closed.
      // Note: the caller should not recycle the instance after calling stop().

[Kinuko Yasuda]

Created attachment 53020 [details]
Patch

[Kinuko Yasuda]

Thanks.

(In reply to comment #17)
> (From update of attachment 53009 [details])
> > +static void derefProxyOnContext(ScriptExecutionContext*, FileStreamProxy* proxy)
> > +{
> > +    proxy->deref();
> Can we add an ASSERT before this line, like:
>        ASSERT(proxy->hasOneRef());
> This way, we can ensure that the caller, i.e., FileReader/FileWriter, has
> already
> released the reference and now it should only has ref count 1 now.

Asserting on ref count makes me a bit nervous, so I added another status flag to allow recycling even after stop() (just in case).

> > diff --git a/WebCore/html/FileStreamProxy.h b/WebCore/html/FileStreamProxy.h
> > +// A proxy module that calls corresponding FileStream methods on the file thread.  Note: you must call stop() before destroying a FileStreamProxy instance, e.g. in the desctructor of an object that holds a reference to the proxy.
> Probably we could say the following for Note part:
>    // ... Note: you must call stop() first and then release the reference to
> the FileStreamProxy instance.

Done.
 
> > +    // Stop() stops any pending tasks, close the file and derefs itself; the caller should not recycle the instance after calling stop().
> It is not needed to repeat stop() in the comment. Also we should use "closes"
> to be consistent with "stops".
> Probably it is better to say "destruct".
>       // Stops the proxy and schedules it to be destructed. All the pending
> tasks will be aborted and the file stream will be closed.
>       // Note: the caller should not recycle the instance after calling stop().

Done.  Also removed the notion about recycling as now it doesn't apply.

[Jian Li]

(In reply to comment #19)
> Asserting on ref count makes me a bit nervous, so I added another status flag
> to allow recycling even after stop() (just in case).
> 

Is there any scenario we want to support reusing FileStreamProxy after calling stop()? If not at present, I'd rather keep the semantic strict because it is kind of confusing. Normally we assume calling something like start() or restart() to revive the instance. But with your new patch, calling any other methods of FileStreamProxy could trigger the reviving and this could make it easy to make the mistake.

[Kinuko Yasuda]

Created attachment 53207 [details]
Patch

[Kinuko Yasuda]

(In reply to comment #20)
> (In reply to comment #19)
> > Asserting on ref count makes me a bit nervous, so I added another status flag
> > to allow recycling even after stop() (just in case).
> 
> Is there any scenario we want to support reusing FileStreamProxy after calling
> stop()? If not at present, I'd rather keep the semantic strict because it is
> kind of confusing. Normally we assume calling something like start() or
> restart() to revive the instance. But with your new patch, calling any other
> methods of FileStreamProxy could trigger the reviving and this could make it
> easy to make the mistake.

Ah ok FileReader has a reason to make it strict. (I changed like that since in FileWriter I didn't see a reason to force the caller to deref the proxy after abort().)
Added the ASSERT as you suggested in your previous review.  Also added a line to nullify the client in stop() to avoid calling back on the client after stop().

[Kinuko Yasuda]

Created attachment 53208 [details]
Rebased

[Kinuko Yasuda]

Created attachment 53209 [details]
Fixed the mess in diff

[Jian Li]

Comment on attachment 53209 [details]
Fixed the mess in diff

r=me

Please fix several minor issues before landing. Thanks.

> diff --git a/WebCore/html/FileStreamProxy.cpp b/WebCore/html/FileStreamProxy.cpp
> +FileStreamProxy::FileStreamProxy(ScriptExecutionContext* context, FileStreamClient* client)
> +    : m_context(context)
> +    , m_client(client)
> +    , m_stream(FileStream::create(this))
> +{
> +    // Holds an extra ref so that the instance will not get deleted while there can be any tasks on the file thread.
> +    ref();
Please add an empty line here for better readability.

> +void FileStreamProxy::stop()
> +{
> +    // Clear the client so that we won't be calling callbacks on the client.
> +    m_client = 0;
Please add an empty line here for better readability.

> diff --git a/WebCore/html/FileStreamProxy.h b/WebCore/html/FileStreamProxy.h
> +    FileThread* fileThread();
Please add an empty line here.

> +    bool m_running;
Please remove this leftover.

[Kinuko Yasuda]

Committed r57749: <http://trac.webkit.org/changeset/57749>

[Darin Adler]

Adam Barth had some comments on this design in bug 41547.

[Kinuko Yasuda]

(In reply to comment #27)
> Adam Barth had some comments on this design in bug 41547.

Thanks, I'm going to open a new issue for that.