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

wait for killfunc completion when shutting down current app #670

Merged
merged 4 commits into from
Jan 19, 2025

Conversation

istyf
Copy link
Contributor

@istyf istyf commented Nov 1, 2024

This PR attempts to fix #669 by reworking how the killFunc is signalled that a shutdown has been requested.

Instead of reading directly from e.binStopCh (that creates a data race with the writer) it closes on a returned shutdown channel that is used from a new mutexed e.stopBin() func. The shutdown channel takes a channel of int, where the int is the "exit code" passed back to the requestor. It isn't actually the exit code of the process, but a code that can be (and currently is) used to os.Exit the entire process if shutdown failed.

The atomic.Bool change is unrelated to the actual issue, but was required to be able to run the unit tests with -race to detect if there were any races between all the channels and goroutine handling going on in this code.

Reading through the issue list there is a good possibility that this PR will fix, or considerably reduce, the problems seen in #640 #492 #334 and do away with the need for #131 .

This PR has been unit tested and development tested on wsl2 linux and an Apple M2 MBP.

@xiantang
Copy link
Collaborator

i will take look into this pr. lets see how to make the logic easy to understand

@istyf
Copy link
Contributor Author

istyf commented Dec 17, 2024

@xiantang Great! Let me know if I can be of any help. I suppose I could add a few comments here and there to make the intention more clear.

runner/engine.go Outdated

eventCh chan string
watcherStopCh chan bool
buildRunCh chan bool
buildRunStopCh chan bool
binStopCh chan bool
binStopCh chan<- chan int
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add coment for binStopCh

@xiantang
Copy link
Collaborator

xiantang commented Jan 17, 2025

Very Good Bidirectional Channels Design

Copy link

codecov bot commented Jan 19, 2025

Codecov Report

Attention: Patch coverage is 72.60274% with 20 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
runner/engine.go 72.60% 17 Missing and 3 partials ⚠️
Files with missing lines Coverage Δ
runner/engine.go 60.74% <72.60%> (-3.58%) ⬇️

... and 11 files with indirect coverage changes

@xiantang xiantang merged commit ad99709 into air-verse:master Jan 19, 2025
8 checks passed
}
})
if ret := <-exitCode; ret != 0 {
os.Exit(ret)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your refactoring is great, I love it.

I would have appreciated if this could be tested.

One solution for this is to define an exiter in the engine.

The exiter could be assigned to os.Exit by default, and overloaded via the tests to return a t.Fatal if the code is not the one expected

What do you think about it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good i will raise a pr for that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wafer-bw added a commit to searchspring/air that referenced this pull request Jan 31, 2025
* chore: bump go to 1.23 (air-verse#650)

* docs: update banner (air-verse#651)

* fix proxy retry loop (air-verse#635)

This addresses two bugs:
1) The loop could exit with an error condition present and fail to
   inform the user.
2) On Windows 11, syscall.ECONNREFUSED is not returned. Instead we
   receive net.OpError with a message similar to:

"unable to reach app: Get "http://localhost:7000/": dial tcp [::1]:7000: connectex: No connection could be made because the target machine actively refused it."

The Fix:
Since the retry loop is short; just 1 second maximum, we retry until no
error happens. If an error does occur, we inform the user.

* Use web page address when making request to reload proxy (air-verse#625)

* Use web page address when making request to reload proxy

Signed-off-by: Alexandra Fishova <yopoyka@yopoyka.com>

* Create event source with just a path because browser can handle address on its own.

Signed-off-by: Alexandra Fishova <yopoyka@yopoyka.com>

---------

Signed-off-by: Alexandra Fishova <yopoyka@yopoyka.com>

* fix the stop_on_error is false configuration does not work (air-verse#555)

* fix color output (air-verse#551)

* Set the Via header in proxy requests to origin and in client responses (air-verse#629)

* Set the Via header in proxy requests to origin and in client responses

* Remove pseudonym and use host only

* feat: Make include_file overrule include_ext, fixes: air-verse#350, base_on: air-verse#358 (air-verse#416)

* feat: Make include_file overrule include_ext

* feat: Make include_file overrule include_ext unittest

* fix: broken code (air-verse#652)

* feat: silent flag in .air.toml, suppressing all air prints (air-verse#641)

* feat: silent flag in .air.toml, suppressing all air prints

useful if the app being watched has structured outputs that are being
piped into subsequent program/s.

* update: add silent to air_example.toml

* FIX: Support working directories containing whitespace (air-verse#646)

* encapsulate paths with quotes to escape spaces

* encapsulate paths with quotes to escape spaces

* preprocess error in test

* Revert "FIX: Support working directories containing whitespace (air-verse#646)" (air-verse#665)

This reverts commit df13da5.

* refactor: remove unnecessary var copy in for (air-verse#682)

* docs: fix badge link in README-zh_cn.md (air-verse#681)

* refactor: simplify test asserts; enable testifylint (air-verse#680)

* Update unclear .profile changes in README.md (air-verse#684)

* Fix: typo in air_example.toml (air-verse#706)

* Implement kill delay on Windows (air-verse#552)

* fix color output

* Implement kill delay on Windows

* Revert the change on master

* Remove extra newline

* Handle absolute paths in config (air-verse#318)

* remove a data race by compiling the exclude regexes in the preprocess phase (air-verse#677)

the regex compilation was done lazily on first access but did not properly synchronize
for being accessed by multiple watcher goroutines

between the option of adding a mutex for something that (should) only ever happen
once and removing the potential for a race, this seems like the better choice

* catch and report start cmd failure rather than panicing (air-verse#676)

* do not report the process as running if it failed to start

* change the return values on error to match what the other platforms produce

* chore: fix test case TestRegexes

* air -h will be document of air

* Update README for air help command

* fix: default config override (air-verse#719)

* Avoid duplicating cors header. (air-verse#727)

* fix: typos (air-verse#728)

* wait for killfunc completion when shutting down current app (air-verse#670)

* wait for killfunc completion when shutting down current app

---------

Co-authored-by: jingdi.zhu <jingdi.zhu@bytedance.com>

* Feature: Show build errors when using proxy (air-verse#725)

* proxy: stream reload and error messages

* proxy: Console log on build failure

* proxy: show build errors in a modal

---------

Co-authored-by: xiantang <zhujingdi1998@gmail.com>

* Add exiter to test os.Exit case (air-verse#729)

authored-by: jingdi.zhu <jingdi.zhu@bytedance.com>

* Fix: using powershell instead of cmd /C (air-verse#708)

---------

Signed-off-by: Alexandra Fishova <yopoyka@yopoyka.com>
Co-authored-by: Rick Yu <m@c6k.app>
Co-authored-by: Greg Dietsche <greg@gregd.org>
Co-authored-by: Alexandra Fishova <yopoyka@yopoyka.com>
Co-authored-by: dengdajun <78243198+pyjcode@users.noreply.github.com>
Co-authored-by: 8LWXpg <105704427+8LWXpg@users.noreply.github.com>
Co-authored-by: dedalusj <dedalusj@gmail.com>
Co-authored-by: zhb127 <1224680008@qq.com>
Co-authored-by: ~ jesse ~ <113159758+jesses-code-adventures@users.noreply.github.com>
Co-authored-by: osteensco <86266589+osteensco@users.noreply.github.com>
Co-authored-by: Oleksandr Redko <oleksandr.red+github@gmail.com>
Co-authored-by: Gourav Kolhatkar <gstar1525@gmail.com>
Co-authored-by: Olivia Bahr <ob.bahr@gmail.com>
Co-authored-by: ZipFile <lin.aaa.lin@gmail.com>
Co-authored-by: Isak Styf <isak@styfsoftware.se>
Co-authored-by: xiantang <zhujingdi1998@gmail.com>
Co-authored-by: Nemanja Milićević <the.nemanja.milicevic@gmail.com>
Co-authored-by: Brandon Bloom <brandon.d.bloom@gmail.com>
Co-authored-by: Nathan Baulch <nathan.baulch@gmail.com>
Co-authored-by: jingdi.zhu <jingdi.zhu@bytedance.com>
Co-authored-by: Polo123456789 <57022222+Polo123456789@users.noreply.github.com>
Co-authored-by: Po <73031052+n1vk@users.noreply.github.com>
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.

Wait for complete current app shutdown before starting a new build run
3 participants