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

Ping remote Agent before shutdown #175

Closed
wants to merge 1 commit into from
Closed

Ping remote Agent before shutdown #175

wants to merge 1 commit into from

Conversation

eberlid
Copy link

@eberlid eberlid commented Feb 4, 2017

Original issue #171

Stopping the agent may cause a 'SocketException'.
Call 'Ping' before, in order to check that the
agent is still alive and waiting for its shutdown.

Stopping the agent may cause a 'SocketException'.
Call 'Ping' before, in order to check that the
agent is still alive and waiting for its shutdown.
@BlythMeister
Copy link
Contributor

i'm having some issues with the Nunit console and a socket error. It seems i'm not the only one,
What is the process to get these reviewed and included into a release?
i see aprox 10-20% of builds fail because nunit is returning a -100 error and that socket exception

@rprouse
Copy link
Member

rprouse commented Mar 29, 2017

Interesting workaround here. I wonder if there is a race condition in the agent where it shuts down before the Stop() call returns. Looking at that may be the more correct fix for this issue. This is more a workaround. @CharliePoole, you've looked at this more, thoughts?

@ChrisMaddock
Copy link
Member

I was about to comment the same as @rprouse - I'd be wary of just suppressing the exception, without fully understanding what's causing it.

Both @BlythMeister and @eberlid have encountered this while running multiple consoles in parallel, as opposed to letting the engine handle the parallelisation. As @CharliePoole wrote on #171 - that's uncommon, and could be bringing it's own issues. Given that two people have now encountered this error in this manner, it seems a likely cause - I don't have the time to dig into it right now, though. 😞

@BlythMeister @eberlid - is there a particular reason you run multiple consoles in parallel, as opposed to passing multiple assemblies to a single console, and letting the console run them in parallel?

@CharliePoole
Copy link
Collaborator

CharliePoole commented Mar 30, 2017

@ChrisMaddock I've seen people do this because they want separate output files. It's kind of funny considering how much effort we have put into merging the output!

That may be an option to add at some point.

If folks use multiple consoles in parallel, I'd suggest using --inprocess if possible.

@BlythMeister
Copy link
Contributor

For me, i think its kind of historical as to why.
We are using FAKE which comes with this functionality.

We essentially call nunit console once per test DLL using fake for the parallel.
I could try and call nunit console once and pass all the dll setting workers to the level of parallel we are after.
@harrisonmeister you got any idea why that might not work?

@harrisonmeister
Copy link

@BlythMeister - yes there is a reason. We have more tests / characters per test DLL combined than a command line argument will allow. We hit the max a while back and had to split them out somehow. Single call to nunit console per test was what fake did and fixed the immediate issue

@BlythMeister
Copy link
Contributor

BlythMeister commented Mar 30, 2017

Well...There is the reason...Glad i didn't spend a day changing our code 😋
I'm interested in the inprocess you mention @CharliePoole does this essentially run the test in console rather than create an agent?

Update: I've looked at fake and it supports setting --process (only to single not inprocess, but this looks like it boils down to the same thing in nunit)

@BlythMeister
Copy link
Contributor

@CharliePoole unfortunately we cannot use the "inprocess" flag, as we get this error:
"The --x86 and --inprocess options are incompatible"

Therefore, i think we are going to need to keep using the agent process, but therefore still have this issue with the socket error.

@BlythMeister
Copy link
Contributor

@rprouse I'm not saying this is the solution to the problem either, if there is another way to prevent the socket exception and therefore error we see i'm all for it! :)

@ChrisMaddock
Copy link
Member

We have more tests / characters per test DLL combined than a command line argument will allow.

Have you tried the file option to get around this, which would allow you to specify your command line in a file?

Regardless, I think it would be nice if running multiple consoles in parallel did work. Thinking it through - the same could happen to us. We have a single CI server running many builds - if two of those happened to hit the NUnit stage at the same time, this could be (a lesser version!) of the same issue. As the builds are independent, passing the assemblies in one go isn't an option. I'm not sure this is the solution we want to put in for this problem however - although until anyone has any time to work on it, maybe it's all we've got.

@eberlid
Copy link
Author

eberlid commented Apr 3, 2017

@ChrisMaddock we are running multiple TeamCity agents on one single machine. It happens that more than one agent is executing tests at the same time.

@ChrisMaddock
Copy link
Member

ChrisMaddock commented May 20, 2017

@eberlid @BlythMeister @harrisonmeister - would be great if you guys could try out @jnm2's fix in #223, which should handle this issue more gracefully, and verify it still solves the problem for your cases.

You can pull the NuGet package from the Appveyor artefacts.
https://ci.appveyor.com/project/CharliePoole/nunit-console/build/3.7.0-ci-03635-pr-223/artifacts

@BlythMeister
Copy link
Contributor

We now run using nunit to handle the parallel, so don't have a setup to test the change in afraid.

@eberlid
Copy link
Author

eberlid commented May 22, 2017

I cherry picked the commits of #223 onto version 3.6.1 and awaiting feedback of our test systems.

@jnm2
Copy link
Collaborator

jnm2 commented May 29, 2017

I'm gonna go ahead and close this since I'm closing #171. Thanks for this; it is appreciated. If anything further turns up please let us know!

@jnm2 jnm2 closed this May 29, 2017
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.

7 participants