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 high CPU usage of TeamCityAdapter #8716

Merged
1 commit merged into from Jan 5, 2021

Conversation

matyasbach
Copy link
Contributor

@matyasbach matyasbach commented Jan 2, 2021

Fixes #8281 and possibly also fixes #3646 and fixes #8167

Exceptions thrown from TeamCityAdapter caused immediate retry of failing operations -> spawning lot of new threads without any delay.

Before

These cases caused high CPU usage:

  • incorrect (nonexistent) TC build ID
  • network or TC server outage

After

CPU usage stays minimal. Build server is queried in defined time intervals even when responding errors or not communicating.

Test methodology

Manually testing TeamCity integration and:

  • simulating network/VPN outages
  • set incorrect build ID

Other build server types could be affected and were not tested.


✒️ I contribute this code under The Developer Certificate of Origin.

Exceptions thrown from TeamCityAdapter caused immediate retry of failing
operations -> spawning lot of new threads without any delay.

Fixes gitextensions#8281
@ghost ghost assigned matyasbach Jan 2, 2021
@codecov
Copy link

codecov bot commented Jan 2, 2021

Codecov Report

Merging #8716 (a1fb666) into master (b304774) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #8716      +/-   ##
==========================================
- Coverage   55.70%   55.69%   -0.01%     
==========================================
  Files         899      899              
  Lines       65055    65055              
  Branches    11733    11733              
==========================================
- Hits        36238    36233       -5     
+ Misses      25982    25981       -1     
- Partials     2835     2841       +6     
Flag Coverage Δ
production 42.68% <0.00%> (-0.01%) ⬇️
tests 94.92% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@RussKie
Copy link
Member

RussKie commented Jan 3, 2021

Thank you.

Will wait for a feedback from users reported the issue before merging.

@RussKie RussKie added the BLOCKED Blocked by another piece of work label Jan 3, 2021
@alastairtree
Copy link

Have tested the teamcity integration and it still works. It prompted me for credentials correctly (for the first time in ages) and handles network connection/dissconnection gracefully on refresh where it seemed more flakey beforehand. So no regressions and and looks like a promising fix. Thank you!

@RussKie RussKie removed the BLOCKED Blocked by another piece of work label Jan 3, 2021
@RussKie
Copy link
Member

RussKie commented Jan 3, 2021

@msftbot merge in 48 hours

@ghost ghost added the status: auto merge label Jan 3, 2021
@ghost
Copy link

ghost commented Jan 3, 2021

Hello @RussKie!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Tue, 05 Jan 2021 13:57:52 GMT, which is in 2 days

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost merged commit 0b32ea3 into gitextensions:master Jan 5, 2021
@ghost ghost added this to the 4.0 milestone Jan 5, 2021
@gerhardol
Copy link
Member

For Jenkins, this PR causes a regression: New builds are not detected.
Quite hard to debug, there are few public Jenkins servers.

@RussKie
Copy link
Member

RussKie commented Feb 4, 2021

this PR causes a regression: New builds are not detected.

Do we need to roll this back for 3.5?

@gerhardol
Copy link
Member

You can manually refresh so there is a workaround.
The share of Jenkins vs TeamCity users decides the urgency...

I will try to investigate and create an issue

@matyasbach
Copy link
Contributor Author

Ups, sorry for regression :'(

And yes, it's really hard to debug. I spent another few hours now trying to debug/fix high CPU bug another way without success.

It (#8281) was introduced by one or both of these PRs - #7452, #7485. I'm not experienced with neither Rx nor Microsoft.VisualStudio.Threading so if anyone can spot what's wrong in BuildServerWatcher.cs or point me in the right direction I'll be happy to try it and test it against TC build server.

@gerhardol
Copy link
Member

AppVeyor likely have the same problem as Jenkins (GetFinishedBuildsSince() is always empty, all builds retrieved in GetRunningBuilds()).
TC and others have the fullDayObservable as a backup.

I do not get any AppVeyor builds with 3.4.3 or 3.3 either, (I have had that for some time, I thought it was after som experiments) and cannot get it working.
Not really any debug possibilities with Jenkins, will try something.

@RussKie
Copy link
Member

RussKie commented Feb 7, 2021

I have no problems with AppVeyor:
image

@gerhardol
Copy link
Member

I have no problems with AppVeyor:

See #8828

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment