automation: only read complete lines before trying to deserialize #15778
Conversation
This file contains 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
When tailing the event log in automation API we currently have nothing that makes sure we read only complete lines. This means if the OS happens to flush an incomplete line for whatever reason (or the Go JSON encoder does, which we're using to write these lines), we might read a line that is incompletely written, and thus will fail to JSON decode it. Since the JSON encoder always writes a newline at the end of each string, we can also make sure that the line we read ends with a newline and otherwise wait for the rest of the line to be written. The library we use in Go provides a convenient setting for this, while in python and nodejs we need to add some code to do this ourselves.
Changelog[uncommitted] (2024-03-26)Bug Fixes
|
Frassle
approved these changes
Mar 25, 2024
|
Guess we need a similar fix in dotnet? |
tgummerer
force-pushed
the
tg/read-complete-lines-only
branch
3 times, most recently
from
8147684
to
2490263
Compare
tgummerer
force-pushed
the
tg/read-complete-lines-only
branch
from
2490263
to
2ffb399
Compare
Yes probably, good call! I'll look into that. |
tgummerer
added a commit
to pulumi/pulumi-dotnet
that referenced
this pull request
Mar 26, 2024
This is the equivalent as pulumi/pulumi#15778 for dotnet. When tailing the event log we currently don't make sure that we read complete lines. Lines can be flushed in the middle by the OS while they are being written down, leading to incomplete JSON. The JSON encoder always ends each line with a newline, so we can stitch the lines back together even if we occasionally read a partly written line.
tgummerer
added a commit
to pulumi/pulumi-dotnet
that referenced
this pull request
Mar 26, 2024
This is the equivalent as pulumi/pulumi#15778 for dotnet. When tailing the event log we currently don't make sure that we read complete lines. Lines can be flushed in the middle by the OS while they are being written down, leading to incomplete JSON. The JSON encoder always ends each line with a newline, so we can stitch the lines back together even if we occasionally read a partly written line.
tgummerer
added a commit
to pulumi/pulumi-dotnet
that referenced
this pull request
Mar 26, 2024
This is the equivalent as pulumi/pulumi#15778 for dotnet. When tailing the event log we currently don't make sure that we read complete lines. Lines can be flushed in the middle by the OS while they are being written down, leading to incomplete JSON. The JSON encoder always ends each line with a newline, so we can stitch the lines back together even if we occasionally read a partly written line.
tgummerer
added a commit
to pulumi/pulumi-dotnet
that referenced
this pull request
Mar 26, 2024
This is the equivalent as pulumi/pulumi#15778 for dotnet. When tailing the event log we currently don't make sure that we read complete lines. Lines can be flushed in the middle by the OS while they are being written down, leading to incomplete JSON. The JSON encoder always ends each line with a newline, so we can stitch the lines back together even if we occasionally read a partly written line.
tgummerer
added a commit
to pulumi/pulumi-dotnet
that referenced
this pull request
Mar 26, 2024
This is the equivalent as pulumi/pulumi#15778 for dotnet. When tailing the event log we currently don't make sure that we read complete lines. Lines can be flushed in the middle by the OS while they are being written down, leading to incomplete JSON. The JSON encoder always ends each line with a newline, so we can stitch the lines back together even if we occasionally read a partly written line.
tgummerer
added a commit
to pulumi/pulumi-dotnet
that referenced
this pull request
Mar 26, 2024
This is the equivalent as pulumi/pulumi#15778 for dotnet. When tailing the event log we currently don't make sure that we read complete lines. Lines can be flushed in the middle by the OS while they are being written down, leading to incomplete JSON. The JSON encoder always ends each line with a newline, so we can stitch the lines back together even if we occasionally read a partly written line.
tgummerer
commented
Mar 26, 2024
sdk/nodejs/automation/stack.ts
Outdated
Comment on lines
126
to
136
| let partialLine = ""; | ||
| lineSplitter.on("line", (line) => { | ||
| let event: EngineEvent; | ||
| try { | ||
| event = JSON.parse(line); | ||
| line = partialLine + line; | ||
| partialLine = ""; | ||
| event = JSON.parse(line); | ||
| callback(event); | ||
| } catch (e) { | ||
| log.warn(`Failed to parse engine event | ||
| If you're seeing this warning, please comment on https://github.com/pulumi/pulumi/issues/6768 with the event and any | ||
| details about your environment. | ||
|
|
||
| Event: ${line}\n${e.toString()}`); | ||
| partialLine += line; | ||
| return; |
There was a problem hiding this comment.
Unfortunately nodejs doesn't seem to include the newline character in the on line events, so we'll have to go off catching exceptions here.
tgummerer
force-pushed
the
tg/read-complete-lines-only
branch
from
fac105a
to
954247d
Compare
|
pulumi/pulumi-dotnet#245 is the corresponding dotnet PR. |
Yeah, we should have a test for Node.js. It doesn't look like /** @internal */
export async function readLines(logPath: string, callback: (event: EngineEvent) => void): Promise<ReadlineResult> {
// ...
} |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 27, 2024
Tentative changelog: ### Features - [docs] Implement constructor syntax examples for every resource in typescript, python, csharp and go [#15624](#15624) - [engine] Send output values with property dependency information to transform functions [#15637](#15637) - [engine] Add a --continue-on-error flag to pulumi destroy [#15727](#15727) - [sdk/go] Make `property.Map` keyed by `string` not `MapKey` [#15767](#15767) - [sdk/python] Improve the error message when depends_on is passed objects of the wrong type [#15737](#15737) ### Bug Fixes - [auto/{go,nodejs,python}] Make sure to read complete lines before trying to deserialize them as engine events [#15778](#15778) - [cli/plugin] Fix installing local language plugins on Windows [#15715](#15715) - [engine] Don't delete stack outputs on failed deployments [#15754](#15754) - [engine] Fix a panic when updating provider version in a run using --target [#15716](#15716) - [engine] Handle that Assets & Archives can be returned from providers without content. [#15736](#15736) - [engine] Fix the engine trying to delete a protected resource caught in a replace chain [#15776](#15776) - [sdkgen/docs] Add missing newline for `Coming soon!` [#15783](#15783) - [programgen/dotnet] Fix generated code for a list of resources used in resource option DependsOn [#15773](#15773) - [programgen/{dotnet,go}] Fixes emitted code for object expressions assigned to properties of type Any [#15770](#15770) - [sdk/go] Fix lookup of plugin and program dependencies when using Go workspaces [#15743](#15743) - [sdk/nodejs] Export automation.tag.TagMap type [#15774](#15774) - [sdk/python] Wait only for pending outputs in the Python SDK, not all pending asyncio tasks [#15744](#15744) ### Miscellaneous - [sdk/nodejs] Reorganize function serialization tests [#15753](#15753) - [sdk/nodejs] Move mockpackage tests to closure integration tests [#15757](#15757)
Merged
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 28, 2024
Tentative changelog: ### Features - [docs] Implement constructor syntax examples for every resource in typescript, python, csharp and go [#15624](#15624) - [docs] Implement YAML constructor syntax examples in the docs [#15791](#15791) - [engine] Send output values with property dependency information to transform functions [#15637](#15637) - [engine] Add a --continue-on-error flag to pulumi destroy [#15727](#15727) - [sdk/go] Make `property.Map` keyed by `string` not `MapKey` [#15767](#15767) - [sdk/nodejs] Make function serialization work with typescript 4 and 5 [#15761](#15761) - [sdk/python] Improve the error message when depends_on is passed objects of the wrong type [#15737](#15737) ### Bug Fixes - [auto/{go,nodejs,python}] Make sure to read complete lines before trying to deserialize them as engine events [#15778](#15778) - [cli/plugin] Fix installing local language plugins on Windows [#15715](#15715) - [engine] Don't delete stack outputs on failed deployments [#15754](#15754) - [engine] Fix a panic when updating provider version in a run using --target [#15716](#15716) - [engine] Handle that Assets & Archives can be returned from providers without content. [#15736](#15736) - [engine] Fix the engine trying to delete a protected resource caught in a replace chain [#15776](#15776) - [sdkgen/docs] Add missing newline for `Coming soon!` [#15783](#15783) - [programgen/dotnet] Fix generated code for a list of resources used in resource option DependsOn [#15773](#15773) - [programgen/{dotnet,go}] Fixes emitted code for object expressions assigned to properties of type Any [#15770](#15770) - [sdk/go] Fix lookup of plugin and program dependencies when using Go workspaces [#15743](#15743) - [sdk/nodejs] Export automation.tag.TagMap type [#15774](#15774) - [sdk/python] Wait only for pending outputs in the Python SDK, not all pending asyncio tasks [#15744](#15744) ### Miscellaneous - [sdk/nodejs] Reorganize function serialization tests [#15753](#15753) - [sdk/nodejs] Move mockpackage tests to closure integration tests [#15757](#15757)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When tailing the event log in automation API we currently have nothing that makes sure we read only complete lines. This means if the OS happens to flush an incomplete line for whatever reason (or the Go JSON encoder does, which we're using to write these lines), we might read a line that is incompletely written, and thus will fail to JSON decode it.
Since the JSON encoder always writes a newline at the end of each string, we can also make sure that the line we read ends with a newline and otherwise wait for the rest of the line to be written.
The library we use in Go provides a convenient setting for this, while in python and nodejs we need to add some code to do this ourselves.
Fixes #15235
Fixes #15652
Fixes #9269 (This is closed already, but never had a proper resolution afaics)
Fixes #6768
It would be nice to add a typescript test here as well, but I'm not sure how to do that without marking the readLines function non-private. But I don't know typescript well, so any hints of how to do that would be appreciated!
Checklist
make tidyto update any new dependenciesmake lintto verify my code passes the lint checkgofumptmake changelogand committed thechangelog/pending/<file>documenting my change