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

[RFC] Remove "end" event #21

Closed
wants to merge 1 commit into from
Closed

[RFC] Remove "end" event #21

wants to merge 1 commit into from

Conversation

clue
Copy link
Member

@clue clue commented Jun 7, 2015

This PR is a RFC – any input is welcome.

Whenever we reach EOF for a stream, we invoke the end() method. This will always emit an "end" and a "close" event.

This PR aims to serve as a discussion basis whether it actually makes sense to keep both.

@@ -26,9 +26,9 @@ public static function pipe(ReadableStreamInterface $source, WritableStreamInter
$source->resume();
});

$end = isset($options['end']) ? $options['end'] : true;
$end = isset($options['close']) ? $options['close'] : true;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the API and its implications here, so I'm just putting this out here for now.

@cboden cboden added this to the v0.5 milestone Sep 10, 2015
@cboden
Copy link
Member

cboden commented Sep 10, 2015

+1

close is always immediately emitted after an end emit. I think it was originally a distinction when calling end to be after jobs were finished but before resources were severed but that's not the case.

@cboden
Copy link
Member

cboden commented Sep 10, 2015

Also flagged this for v0.5 as it's technically an API break.

One scenario I thought of is that since end is emitted before close it could be an event priority hook for some people...I can't think of a scenario as the stream state doesn't change between events, but I suppose it could be a possible case.

@clue
Copy link
Member Author

clue commented Sep 16, 2015

close is always immediately emitted after an end emit. I think it was originally a distinction when calling end to be after jobs were finished but before resources were severed but that's not the case.

Exactly my point, looks like a half-baked feature which never quite made it.

After looking up Node.js's documentation I stumbled upon this:

A related blog post related to the stream2 API also includes this:

Event: 'end'
Emitted when the stream has received an EOF (FIN in TCP terminology). Indicates that no more 'data' events will happen. If the stream is also writable, it may be possible to continue writing.

As such, afaict it looks like the end event is related to half-open duplex streams. This is a feature that we currently do not support, I've just filed #27 for this.

If this is indeed the only use case, I'd suggest removing the end event.

@clue
Copy link
Member Author

clue commented Mar 5, 2016

To be clear: I'm currently 👎 on getting this in, I'm merely hoping to spark some discussion.

IMHO we should look into #27 instead. In this case we could probably make proper use of the end event in future versions.

@clue
Copy link
Member Author

clue commented Dec 19, 2016

See #59 instead.

@clue clue closed this Dec 19, 2016
@clue clue deleted the no-end branch December 19, 2016 23:31
@clue clue removed this from the v0.5.0 milestone Jan 18, 2017
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 this pull request may close these issues.

2 participants