FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream_base: dispatch reqs in the stream impl by indutny · Pull Request #1558 · nodejs/node · GitHub

/ node Public

stream_base: dispatch reqs in the stream impl - #1558

Closed
indutny wants to merge 2 commits into
nodejs:v1.xfrom
indutny:fix/gh-1512
ConversationCommits (2)ChecksFiles changed
Closed

stream_base: dispatch reqs in the stream impl#1558
indutny wants to merge 2 commits into
nodejs:v1.xfrom
indutny:fix/gh-1512

Conversation

indutny commented Apr 29, 2015

Copy link
Copy Markdown
Member

Dispatch requests in the implementation of the stream, not in the code
creating these requests. The requests might be piled up and invoked
internally in the implementation, so it should know better when it is
the time to dispatch them.

In fact, TLS was doing exactly this thing which led us to...

Fix: #1512

indutny mentioned this pull request Apr 29, 2015
Closed

indutny commented Apr 29, 2015

Copy link
Copy Markdown
Member Author

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/635/

mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 29, 2015

Fishrock123 commented Apr 29, 2015

Copy link
Copy Markdown
Contributor

@indutny into v1.x? Shouldn't this target master and then be backported?

indutny commented Apr 29, 2015

Copy link
Copy Markdown
Member Author

Yeah, right. I'm bad at this thing.

stream_base: dispatch reqs in the stream impl
5138d07 
Dispatch requests in the implementation of the stream, not in the code
creating these requests. The requests might be piled up and invoked
internally in the implementation, so it should know better when it is
the time to dispatch them.

In fact, TLS was doing exactly this thing which led us to...

Fix: nodejs#1512
tls: ensure no synchronous callbacks
af885a4 
Make sure that no WriteItem's callback will be invoked synchronously.
Doing so may lead to the use of uninitialized `req` object, or even
worse use-after-free in the caller code.

Fix: nodejs#1512

silverwind commented Apr 29, 2015

Copy link
Copy Markdown
Contributor

#1512 looks to be fixed with the last commit, probaby best to run another CI now. I think this fix may have to go in without a test, unless someone has an idea.

indutny commented Apr 29, 2015

Copy link
Copy Markdown
Member Author

I'll probably do it a better way... will reopen this PR to target a master branch.

silverwind commented Apr 29, 2015

Copy link
Copy Markdown
Contributor

Does the target branch really matter? With our manual merge process, it shouldn't have any significance :)

Fishrock123 commented Apr 29, 2015

Copy link
Copy Markdown
Contributor

@silverwind means the CI will work properly. (see #1560)

indutny closed this Apr 29, 2015
indutny deleted the fix/gh-1512 branch April 29, 2015 18:57
indutny mentioned this pull request Apr 29, 2015
Closed
B020239 mentioned this pull request Mar 9, 2024
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL