This repository has been archived by the owner on May 21, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Avoid closing the stream if server still open #47
Avoid closing the stream if server still open #47
Changes from all commits
52b74e6
e97227e
5963044
4ab5962
afff315
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a separate event for the stream "ending" in one direction but not the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm not sure if I'm getting what is the concern here 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently this events measn the whole stream has ended - and will in prtactice be called once. But now it either will mean "the writing direction is closed /ended" or will mean "either the writing or reading direction has ended".
Given that it seems like it will be beneficial to close one or the other and continue getting other/
data
events- it seems beneficial to also be able ot recognise when one or the other direction has ended.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's not. The behavior remains. The
stream.on('end', () => {})
will be triggered only once, when the stream is closed (for both writing and reading).And I see no need for
ending
event since that moment happens exact after callingstream.end()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, but I think the bigger problem is that I personally was mistaken in my understanding of how this is suppsoed to work.
Having taken some time to look at some nodejs exampels it seems to me like
.end()
the method only closes the writing part and theend
event only signals closing thereading
part.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been trying to figure out what happens if you try to write to a stream that that was readonly ... and ... I am not certain what happens as I feel the current examples are not complicated enough :(
And I have been trying to not try to write completely new examples just to get them wrong :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it isn't a good idea to look at the nodejs's docs/examples to get an understanding of how our implementation works 🤷 .
Yes, we have a kind of similar API, but it's not the same and in our docs, we're pretty explicit on how it is supposed to work. Like
stream.end
signals that the client finished the sending andstream.on('end', () => {})
happens when the stream closes.And I believe our current behavior
on
's end is better in k6's context since it could help our users to put some checks there since we declare that this is the moment when the stream finished and everything that should happen is happen or not 😅I might think that here you mean two different cases:
The first case is about the stream being always read-only (a.k.a server-side stream). In that case, we could learn it from the definitions, I believe, and just return trigger an error (or write a log that there was an attempt to write to read-only stream).
The second case is more interesting 🤔 , but I'm not sure that it is realistic. It's the case when the server finishes the stream while the client still wants to send data. On the other side, I believe even in that case, the last word is on the server to report that it's all done (
io.EOF
in the read from server part).