Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calling .then() after .pipe() causes "write after end error" #1277

Closed
jedwards1211 opened this issue Sep 21, 2017 · 11 comments
Closed

Calling .then() after .pipe() causes "write after end error" #1277

jedwards1211 opened this issue Sep 21, 2017 · 11 comments

Comments

@jedwards1211
Copy link
Contributor

I get that .then() is designed to send the request when not using .pipe().

But how about making it so that after .pipe() is called, .then() just waits for the request to finish instead of trying to write again?

@kornelski
Copy link
Contributor

You can't use both. These are mutually exclusive features and can't work together.

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Sep 21, 2017

@pornel is there some technical reason that code like the following would be impossible?

constructor() {
  this.once('pipe', () => this.isPiping = true)
}
then(resolve, reject) {
  if (this.isPiping) {
    this.once('response', resolve)
    if (reject) this.once('error', reject)
  } else {
    this.end(resolve, reject)
  }
}

jedwards1211 added a commit to jedwards1211/superagent that referenced this issue Sep 21, 2017
@jedwards1211
Copy link
Contributor Author

@pornel it is indeed possible, I have made a PR complete with tests.

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Sep 21, 2017

@pornel in fact, if you ask me, it would be better if we could also call .end() after .pipe() and it would merely register the callbacks instead of complaining that it was ended twice. I don't see why the API for getting a response or error should be so different in the .pipe() case.

@kornelski
Copy link
Contributor

Ah, sorry, I assumed you've meant pipe for downloading the request.

So this may work, although I have concerns:

  • can there be race conditions? Is it guaranteed that pipe event is called immediately and sychronously? It'd be a nasty bug if piping started after call to .then().

  • people often try req.get(url).pipe(output).end() expecting this to work. I'm worried about creating confusion where pipe() + end() works in one way, but not the other. And I'm worried that supporting piping and buffering at the same time will be hard (it's already hairy with gzip and async parsers).

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Sep 21, 2017

@pornel

Is it guaranteed that pipe event is called immediately and sychronously?

I haven't found official documentation but it seems to?

var PassThrough = require('stream').PassThrough

var a = new PassThrough()
var b = new PassThrough()
b.on('pipe', () => console.log('pipe'))

console.log('before')
a.pipe(b)
console.log('after')
$ node temp.js
before
pipe
after

All I see in the docs is

The 'pipe' event is emitted when the stream.pipe() method is called on a readable stream

Doesn't say when the event is emitted...

I don't know much about how the buffering works. Are you saying that's only a concern if we support .pipe().end(), or is that a problem with .pipe().then() as well?

@kornelski
Copy link
Contributor

I mean pipe client->server is OK, but pipe server->client is hard.

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Sep 23, 2017

@pornel I'm a bit confused what you mean, are you saying if we support awaiting a request that we've piped something into, you want to support awaiting a response we've piped into something as well?

@kornelski
Copy link
Contributor

Yes. There are two kinds of pipes in superagent, and supporting end/then with only one of them creates inconsistency. Currently superagent doesn't support combination of any pipe with end/then, which may not be very useful, but at least it's a simple rule and is consistent.

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Sep 26, 2017

@pornel I have noticed intermittent failures with awaiting a request that I piped data into now...maybe the pipe event isn't synchronous.

@kornelski
Copy link
Contributor

The potential for race condition is worrying, so I'm going to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants