Skip to content

Bugfix/create server resources on function creation #657

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

Conversation

simschla
Copy link
Contributor

@simschla simschla commented Aug 4, 2020

This the pull request ultimately intended to be fixing #651

The expected problem of "writing to the build dir on step creation" does not appear to be the problem. It seems to me that it is a lifecycle issue.

I've added some debug logging and this results in the following output for my integration test reproducing this issue:

> Task :clean

> Task :spotlessMytypescript
[2020-08-04T07:38:02.096] creating formatter function (starting server)
[2020-08-04T07:38:02.098] preparing node server
[2020-08-04T07:38:02.099] running npm install
[2020-08-04T07:38:05.692] npm install finished
[2020-08-04T07:38:06.259] formatting String 'export class MyVeryOwnControllerWithARatherLongNam[...]' in file '/private/var/folders/2q/sbnkqx013ljc3xbv5tnk8yh8000141/T/junit1671766557151280267/test.ts'
[2020-08-04T07:38:06.444] formatting String 'export class MyVeryOwnControllerWithARatherLongNam[...]' in file '/private/var/folders/2q/sbnkqx013ljc3xbv5tnk8yh8000141/T/junit1671766557151280267/test.ts'
[2020-08-04T07:38:06.452] Closing formatting function (ending server).

> Task :spotlessMytypescriptCheck FAILED
[2020-08-04T07:38:06.715] formatting String 'export class MyVeryOwnControllerWithARatherLongNam[...]' in file '/private/var/folders/2q/sbnkqx013ljc3xbv5tnk8yh8000141/T/junit1671766557151280267/test.ts'
Step 'prettier-format' found problem in 'test.ts':
java.net.ConnectException: Connection refused (Connection refused)

It shows that the server is started at the beginning of spotlessMytypescript and ended/closed at the end of spotlessMytypescript again. But then spotlessMytypescriptCheck is triggered (which does not prepare a new server instance) but tries to format using the "old" formatting function used in spotlessMytypescript (which has already been closed).

Is this a problem that my step is considered "still ready" although it isn't or is this a problem of "closing formatting function too early"?

…ds to be recreated if it is ever used again.
@nedtwigg
Copy link
Member

nedtwigg commented Aug 5, 2020

You're a genius! This was a classic "use after free" style of bug, with an easy one-line fix. The bug was for all "FormatterFunc.Closeable", but the npm steps are the only ones that use that functionality. Next up is a test which shows that it is fixed.

@nedtwigg
Copy link
Member

nedtwigg commented Aug 5, 2020

Part of this is also the 5.x refactor. Previously, apply and check were both implemented by the same task, so the formatter was only ever created once. Now that formatter is potentially constructed twice, the bug is exposed. The bug was never visible to maven users.

I updated the changelogs, this looks fixed to me. I'll let you decide if you want to do anything else (maybe silence the logging?) and then you can press merge.

…r-resources-on-function-creation

# Conflicts:
#	plugin-gradle/CHANGES.md
@simschla simschla marked this pull request as ready for review August 5, 2020 14:38
@nedtwigg nedtwigg merged commit f59f3fa into diffplug:main Aug 5, 2020
@simschla
Copy link
Contributor Author

simschla commented Aug 6, 2020

Awesome, thank you for cleaning everything up. 👍

@simschla simschla deleted the bugfix/create-server-resources-on-function-creation branch August 6, 2020 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants