Skip to content

Ensure relative order of DAP messages and repl elements; #33822; #79196#81196

Closed
dgozman wants to merge 1 commit intomicrosoft:masterfrom
dgozman:fix-79196-33822
Closed

Ensure relative order of DAP messages and repl elements; #33822; #79196#81196
dgozman wants to merge 1 commit intomicrosoft:masterfrom
dgozman:fix-79196-33822

Conversation

@dgozman
Copy link
Contributor

@dgozman dgozman commented Sep 19, 2019

Three fixes:

  • handle each DAP message in a separate task to ensure that all microtasks run in between;
  • append repl elements in sequential order by chaining their async processing;
  • fire onDidChangeREPLElements directly in ReplModel to ensure it is fired at the right time.

@isidorn What do you think about this approach? Seems to resolve all test cases for me:

  • console.log([1, 2]); 3
  • console.log([1, 2]); console.log(3); setTimeout(() => console.log(5), 0); 4
  • setTimeout(() => console.log(5), 0); Array(1000).fill(0)

@dgozman
Ensure relative order of DAP messages and repl elements; microsoft#33822
8b54202 
; microsoft#79196

Two fixes:
- handle each DAP message in a separate task to ensure that all
  microtasks run in between;
- append repl elements in sequential order by chaining their async
  processing.
@kieferrm kieferrm requested review from isidorn and weinand and removed request for isidorn September 19, 2019 21:24
@isidorn isidorn added this to the September 2019 milestone Sep 20, 2019
@isidorn
Copy link
Collaborator

isidorn commented Sep 20, 2019

@dgozman thanks for the PR.
Here's some inital feedback:

  • There are conflicts with the master branch which make it harder for me to nicely review this
  • As you mention this PR seems to do multiple things, thus increasing its complexity. Could we maybe seperate this PR into smaller PRs so it is easier for us to review and merge. If the separate parts make sense on their own of course
  • I do not like that the repl events is now half half fired from the model and from the session. Can we move everything to the model? And the sesssion just re-exposes that event? Probably the addReplExpression will aslo need to fire on the session side, but that can be the only corner case
  • Handling all the DAP messages with a timeout of 0 feels a bit like a hack to me, and just out of luck works. I might be wrong, but I would like if we get that as an independent PR.
  • Instead of using slots it might be easier to use some of our async utilites. Maybe the Sequencer is what you are looking for. There is other cool stuff there.

Thanks again and let me know what you think about my feedback.

This was referenced Sep 24, 2019
Merged
Merged
@isidorn
Copy link
Collaborator

isidorn commented Sep 25, 2019

Since you have created separate PRs I will close this one. Hope that is ok.

@isidorn isidorn closed this Sep 25, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@isidorn isidorn Awaiting requested review from isidorn

@weinand weinand Awaiting requested review from weinand

Assignees

@isidorn isidorn

Labels

None yet

Projects

None yet

Milestone

September 2019

Development

Successfully merging this pull request may close these issues.

3 participants

@dgozman @isidorn @weinand