470716 - Make the close-output-stream-at-completion behavior of nsAsyncStreamCopier configurable

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[Jeff Walden [:Waldo]]

nsAsyncStreamCopier copies one stream to another.  Currently the "another" stream is closed when the copy completes; this isn't helpful in cases where you might want to continue writing to that stream once the copy completes.  For example, httpd.js writes out the body of a request this way, but if it wanted to support keep-alive connections it'd have to not use nsAsyncStreamCopier because then the keep-alive connection would be closed.  Instead, this close-when-done behavior should be configurable when the init method on the copier is called.

Comment 1

[Michal Novotny [:michal]]

Attachment: patch that implements close-when-done option

Comment 2

[Jeff Walden [:Waldo]]

You need to bump the IID of nsIAsyncStreamCopier if you're changing its methods or attributes...

Comment 3

[Michal Novotny [:michal]]

Attachment: patch v2, IID of nsIAsyncStreamCopier changed

Comment 4

[Benjamin Smedberg]

Comment on attachment 357223 [details] [diff] [review]
patch v2, IID of nsIAsyncStreamCopier changed

I am not a necko peer. The API change looks fine, but I don't think we want to take it on the 1.9.1 branch.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So...  The documentation makes it sound like the booleans only affect end-of-copy behavior, but they also affect error condition behavior, no?  Is that what we want?

Also, you'll want to update devmo to whatever the result ends up looking like here.

Comment 6

[Michal Novotny [:michal]]

(In reply to comment #5)
> So...  The documentation makes it sound like the booleans only affect
> end-of-copy behavior, but they also affect error condition behavior, no?  Is
> that what we want?

I think that we want the same behaviour when copying succeeds and when it fails, no? I'll correct the documentation...

> Also, you'll want to update devmo to whatever the result ends up looking like
> here.

You mean page https://developer.mozilla.org/En/NsIAsyncStreamCopier ?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I think that we want the same behaviour when copying succeeds and when it
> fails, no?

Are we still going to propagate the error condition to the caller somehow?

> You mean page https://developer.mozilla.org/En/NsIAsyncStreamCopier ?

No, there's at least one page which has a code snippet example using the stream copier.  Sadly, the search engine on devmo seems to not search code snippets or something...  If you can't find it, let me know and I'll grovel through my history looking for it.  :(

Comment 8

[Michal Novotny [:michal]]

(In reply to comment #7)
> > I think that we want the same behaviour when copying succeeds and when it
> > fails, no?
> 
> Are we still going to propagate the error condition to the caller somehow?

I've tested it and error conditions are propagated to the caller via nsIRequestObserver provided by caller. But I've realized that my change in nsAsyncStreamCopier::Cancel() is completely wrong. Cancelling is done by closing both streams and there is now no other option to stop copying. To be able to cancel copying without closing streams I need to make class nsAStreamCopier public, implement new method Cancel() and use it in nsAsyncStreamCopier.

Or of course I can change the logic so that streams are always closed when some error occurs on input/output stream or when nsAsyncStreamCopier::Cancel() is called.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Given comment 8, are you still waiting on my review?

Comment 10

[Michal Novotny [:michal]]

Actually I would like to know which solution is preferred. I like more the one when streams are closed neither on error nor when Cancel() is called.

Comment 11

[Jeff Walden [:Waldo]]

I think I agree; if an error happens, stop trying to fix the problem and let the calling code deal.

Comment 12

[Michal Novotny [:michal]]

Attachment: patch v4, fixed nsAsyncStreamCopier::Cancel()

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 362075 [details] [diff] [review]
patch v4, fixed nsAsyncStreamCopier::Cancel()

Shouldn't cancel null out mCopierCtx?  That is, swap() mCopierCtx into copierCtx instead of assigning.

I'd like to see some unit tests here.

And the devmo documentation needs to be updated, as I said.

Other than that, looks good.

Comment 14

[Michal Novotny [:michal]]

Attachment: patch v5, test added

(In reply to comment #13)
> (From update of attachment 362075 [details] [diff] [review])
> Shouldn't cancel null out mCopierCtx?  That is, swap() mCopierCtx into
> copierCtx instead of assigning.

It is null out in nsAsyncStreamCopier::Complete().

> I'd like to see some unit tests here.

Please review the test.

> And the devmo documentation needs to be updated, as I said.

Using google I found 3 pages that need to be modified:

  https://developer.mozilla.org/en/nsIAsyncStreamCopier
  https://developer.mozilla.org/en/Code_snippets/Miscellaneous
  https://developer.mozilla.org/en/Components.isSuccessCode

I'll update them after I get sr+, ok?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> It is null out in nsAsyncStreamCopier::Complete().

Yes, I know it is.  But the current code allows Cancel() to happen more than once and for later Cancel() calls to override the previous ones' nsresult.  This is wrong, no?

> Please review the test.

Is the 0x81234567 result something meaningful?  Or just an error code made up for this test?  If the former, does it live in Components.results?  If so, use it from there; if not, can you add a comment saying what it really is?

Instead of the

  if (x) y = true;
  else y = false;

pattern, why not just use:

  x = y;

or 

  x = (y != 0);

for numeric y if you don't want to rely on xpconnect doing the right thing for you?

The if conditions in tests 9-12 should be subtracting 9 from test_nr, not 5.

In the try/catches checking for closed source/sink, would it make sense to only catch the exception if it's NS_BASE_STREAM_CLOSED?  Or do you get other exceptions there too?

> I'll update them after I get sr+, ok?

Sounds good.

Comment 16

[Michal Novotny [:michal]]

Attachment: patch v6

(In reply to comment #15)
> Yes, I know it is.  But the current code allows Cancel() to happen more than
> once and for later Cancel() calls to override the previous ones' nsresult. 
> This is wrong, no?

The second call will fail in nsAStreamCopier::Cancel() with error NS_ERROR_FAILURE so previous nsresult won't be overriden. But nsAsyncStreamCopier::Cancel() will return NS_OK. That's ok, no?

> In the try/catches checking for closed source/sink, would it make sense to only
> catch the exception if it's NS_BASE_STREAM_CLOSED?  Or do you get other
> exceptions there too?

NS_BASE_STREAM_CLOSED is thrown only when testing EOF on sink (test 5-8). In other cases is thrown error code passed via Cancel() or CloseWithStatus(). I'm not sure if it makes sense to test it...

Comment 17

[Boris Zbarsky [:bzbarsky]]

> The second call will fail in nsAStreamCopier::Cancel() with error
> NS_ERROR_FAILURE so previous nsresult won't be overriden. But
> nsAsyncStreamCopier::Cancel() will return NS_OK. That's ok, no?

Hmm.  OK, but very non-obvious.  I think just nulling out the context once it's been used is much clearer.

Sounds ok on the exceptions.

Comment 18

[Michal Novotny [:michal]]

Attachment: patch v7

(In reply to comment #17)
> Hmm.  OK, but very non-obvious.  I think just nulling out the context once it's
> been used is much clearer.

OK

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 366606 [details] [diff] [review]
patch v7

Looks good.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/57dc99fc19e6

Still need those devmo docs updated.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Oh, and for future reference, patches with the checkin comment and user already in them are very much appreciated for checkin-needed bugs...

Comment 22

[Michal Novotny [:michal]]

> Still need those devmo docs updated.

I've already updated it on the 10th March.

Comment 23

[Michal Novotny [:michal]]

(In reply to comment #21)
> Oh, and for future reference, patches with the checkin comment and user already
> in them are very much appreciated for checkin-needed bugs...

Sorry, I'm not sure I understand what you mean.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> I've already updated it on the 10th March.

Oh, I see.  I fixed one major factual inaccuracy in the result ("last three arguments" no longer refererred to the last three arguments), but what that code now needs is probably two versions: one for this new interface, one for the old.  With notes as to which one applies to which Firefox versions and how you can tell.  As it stands, that page just became useless to anyone working on Firefox 3 or 3.5 extensions...

> Sorry, I'm not sure I understand what you mean.

Please produce your patches for checkin either using hg export or by attaching diffs directly from your .hg/patches directory; make sure the patches have you set as the user and have a checkin comment set.

Comment 25

[Michal Novotny [:michal]]

> but what that code now needs is probably two versions: one for this new
> interface, one for the old.

I've changed it slightly, hope it is OK now.

> Please produce your patches for checkin either using hg export or by attaching
> diffs directly from your .hg/patches directory; make sure the patches have you
> set as the user and have a checkin comment set.

Thanks for clarification.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> I've changed it slightly, hope it is OK now.

Looks good.

Comment 27

[Eric Shepherd [:sheppy]]

Someone had already (mostly) updated the documentation. I gave the nsIAsyncStreamCopier documentation a buffing up, but it was already accurate as far as I can tell:

https://developer.mozilla.org/En/NsIAsyncStreamCopier