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

fix: normalize and replace line separator in node task lock file #19092

Merged

Conversation

mcollovati
Copy link
Collaborator

Normalizes and replaces line separators potentially present in the process command line before writing the lock file, so that it will contain only two lines. Same normalization is applied when getting the command line for the process identified by the pid in the lock file before comparing it with the value stored in the lock file.

Fixes #19091

Normalizes and replaces line separators potentially present in the process
command line before writing the lock file, so that it will contain only two
lines. Same normalization is applied when getting the command line for the
process identified by the pid in the lock file before comparing it with
the value stored in the lock file.

Fixes #19091
Copy link

sonarcloud bot commented Apr 3, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@mcollovati
Copy link
Collaborator Author

Manually tested this way:

Without the patch, run maven with a parameter that spans over multiple rows, e.g.

mvn -Dxxxx="sdfdsf
dfgdfgdf
dfgdfgdfg
dfgdfg
" process-resources

In the console log, you should see the error Error releasing lock file .... and the lock file is not deleted.

With the patch, the error is not shown.

Additional test:

  1. Put a break point in NodeTasks.writeLockFile on the Files.write() instruction
  2. Run the above maven command in debug mode (mvnDebug)
  3. When the debugger reaches the breakpoint, proceed until the lock file is written and then wait
  4. On another terminal, run the same maven command, without debug mode. On the console, you should see something like Waiting for a previous instance of NodeTasks (pid: 84845) to finish...
  5. Detach the debugger so that the command on the first terminal can complete
  6. Check that also the command launched on the other terminal completes without errors and the lock file is deleted

Copy link

github-actions bot commented Apr 3, 2024

Test Results

1 099 files  ±0  1 099 suites  ±0   1h 24m 7s ⏱️ - 1m 57s
6 938 tests ±0  6 889 ✅ ±0  49 💤 ±0  0 ❌ ±0 
7 302 runs   - 2  7 241 ✅  - 2  61 💤 ±0  0 ❌ ±0 

Results for commit de6d624. ± Comparison against base commit eb30c25.

@mcollovati
Copy link
Collaborator Author

So far, I have no good idea on how to implement a proper test for this change

@caalador
Copy link
Contributor

caalador commented Apr 4, 2024

I don't see a good way to test this either...

@mcollovati mcollovati merged commit 769e3db into main Apr 4, 2024
26 checks passed
@mcollovati mcollovati deleted the issues/19091_node_task_lockfile_command_line_newline branch April 4, 2024 07:18
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.4.0.alpha21 and is also targeting the upcoming stable 24.4.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.flow-node-tasks.lock can contain more than the two expected lines due to CLI args with line-breaks
3 participants