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

Remove "process already captured" errors. #9337

Closed
wants to merge 1 commit into from

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Sep 22, 2020

This has been a very annoying source of CI failures. If it were technically more correct I'd say we should keep it around and attempt to fix the underlying issue, but I do not think it is. In most cases, we are either setting up on the same object (in which case no need to release any existing handlers) or we are providing a custom MockProcess object which really won't ever need capturing.

This has been a very annoying source of CI failures. If it were
technically more correct I'd say we should keep it around and attempt
to fix the underlying issue, but I do not think it is. In most cases,
we are either setting up on the same object (in which case no need to
release any existing handlers) or we are providing a custom
`MockProcess` object which really won't ever _need_ capturing.
@rwjblue rwjblue force-pushed the remove-process-already-captured branch from afc1141 to 3fac7fa Compare September 22, 2020 20:08
@ro0gr
Copy link
Contributor

ro0gr commented Sep 22, 2020

If I remember correctly, in case if ember-cli would capture the process twice, it would not properly dispose stuff from the first capture. This was a reason why I've introduced this exception(sorry for a constant problems with this!).

Is it some kind of sporadic failures of the ember-cli's own CI, or does it cause some issues for external projects?

@rwjblue
Copy link
Member Author

rwjblue commented Sep 22, 2020

Is it some kind of sporadic failures of the ember-cli's own CI

Ya, it is due to us using tests/helpers/ember.js to invoke the CLI from within the same process many times (some of our tests use execa + a new process but others use that helper).

@rwjblue
Copy link
Member Author

rwjblue commented Sep 22, 2020

@ro0gr
Copy link
Contributor

ro0gr commented Sep 22, 2020

Seems like the failure occurrs only when the tests/helpers/ember.js is used? If such, can we maybe release( only when the test helper is used? This way, we'd be resilent to the test setup, and leave the userland code guard in place?

@ro0gr
Copy link
Contributor

ro0gr commented Sep 22, 2020

If that sounds reasonable, I can take a look at it this week.

@ro0gr
Copy link
Contributor

ro0gr commented Sep 23, 2020

If I remember correctly, in case if ember-cli would capture the process twice, it would not properly dispose stuff from the first capture.

I've just checked capture-exit, and seems like double capture should not really cause the issue.

Though, in my taste, it still might be a good idea to preserve the exception in order to avoid accidental double capture coming from the /lib, your change looks good to me.

@rwjblue
Copy link
Member Author

rwjblue commented Sep 23, 2020

I've just checked capture-exit, and seems like double capture should not really cause the issue.

Ya, capture-exit always captures whatever process is (and we do not pass the value that is passed in to us).

@rwjblue
Copy link
Member Author

rwjblue commented Sep 23, 2020

If that sounds reasonable, I can take a look at it this week.

Ya, I'm happy to try that before completely removing (though I think the part of the changes here that are "if the process is the same thing, carry on" are probably still fine) if you have the time to look into it.

FWIW, I also considered adding a .release() call in the global afterEach we have in tests/runner.js. This would at least prevent "knock on failures" (where every subsequent test fails once any test fails).

@ro0gr
Copy link
Contributor

ro0gr commented Sep 23, 2020

Thanks, I'd definitely give it a try!

"if the process is the same thing, carry on" are probably still fine

Yes, it's clever, and it makes sense in terms of the capture-exit.

However, due to the requirement for the will-interrupt-process to be invoked at the very begining of the process, from the ember-cli's /lib perspective, I believe it doesn't make sense to allow it being called twice within the same process. It just feels pointless to invoke it twice. Though I realize, it may be just a matter of my bias.

adding a .release() call in the global afterEach we have in tests/runner.js

Ya, that's an option. I think the drawback of this is that, tests/helpers/ember is a public API of ember-cli(I believe). So someone using this test helper may experience the same issue in their addon's test suite.

@ro0gr
Copy link
Contributor

ro0gr commented Sep 23, 2020

A side note.

I've just checked few more builds with this failure:

each time, the same test triggers the initial error:

ember new npm blueprint with old version

This is suspicious at least...

@rwjblue
Copy link
Member Author

rwjblue commented Sep 24, 2020

Ya, that's an option. I think the drawback of this is that, tests/helpers/ember is a public API of ember-cli(I believe). So someone using this test helper may experience the same issue in their addon's test suite.

Ya, totally agree with you here. I absolutely would prefer to get this squared away without sacrificing strictness.

ro0gr pushed a commit to ro0gr/ember-cli that referenced this pull request Oct 1, 2020
for some reason we have sporadic test failures on CI, like:

 > process already captured

This is quite hard to reproduce and the root cause
of such a failure is not clear at the moment.

This commit is an attempt to provide an alternative to ember-cli#9337,
so with this we do not touch `will-interrupt-process`,
and try to fix it in the tests land only.
ro0gr added a commit to ro0gr/ember-cli that referenced this pull request Oct 1, 2020
for some reason we have sporadic test failures on CI, like:

 > process already captured

This is quite hard to reproduce and the root cause
of such a failure is not clear at the moment.

This commit is an attempt to provide an alternative to ember-cli#9337,
so with this we do not touch `will-interrupt-process`,
and try to fix it in the tests land only.
@rwjblue rwjblue closed this Oct 12, 2020
@rwjblue rwjblue deleted the remove-process-already-captured branch October 12, 2020 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants