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

Set BridgeResourceFolder at Bridge startup #337

Merged
merged 2 commits into from
Sep 16, 2015

Conversation

roncain
Copy link
Contributor

@roncain roncain commented Sep 15, 2015

Prior to this change, the BridgeResourceFolder was specified by
BridgeClient when tests ran. However, this was not a valid
strategy for x-platform use, otherwise both client and Bridge
would need to share a x-OS folder.

With this change, Bridge.exe is required to have a BridgeResourceFolder
when it is asked to start, and the BridgeClient no longer alters that.

Also adds a 'reset' option to Bridge.exe so that when tests complete
the Bridge can be asked to release all remote resources. They will
be reacquired the next time Bridge requests are made.

Also move the Http DELETE handling to the ResourceController where it
really belonged. An Http DELETE to the /resource endpoint releases all
resources.

Fixes #334

Prior to this change, the BridgeResourceFolder was specified by
BridgeClient when tests ran.  However, this was not a valid
strategy for x-platform use, otherwise both client and Bridge
would need to share a x-OS folder.

With this change, Bridge.exe is required to have a BridgeResourceFolder
when it is asked to start, and the BridgeClient no longer alters that.

Also adds a 'reset' option to Bridge.exe so that when tests complete
the Bridge can be asked to release all remote resources.  They will
be reacquired the next time Bridge requests are made.

Also move the Http DELETE handling to the ResourceController where it
really belonged.  An Http DELETE to the /resource endpoint releases all
resources.

Fixes dotnet#334
pushd %~dp0..\..\..\..\bin\wcf\tools\Bridge
echo Invoking Bridge.exe -reset %* ...
call Bridge.exe -reset %*
popd
echo The Bridge was left running because BridgeKeepRunning is true
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, CleanupWcfTestService.cmd is called immediately after a run happens. Do we necessarily want to get rid of all resources that the bridge is holding?

My flow of work is typically the following:

  • run startBridge.cmd
  • run build.cmd to look at what's wrong
  • make changes
  • rebuild.

Sometimes, in the "rebuild" step I run build.cmd, other times I build the project and test directly.
My suspicion is that if someone has left the bridge running, we should probably leave any resources opened alone, and rely on the developer to either exit the Bridge or call reset manually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CleanupWcfTestService.cmd is the script called to clean up all resources -- that's its purpose. In this case, cleanup means remove all certs and firewall rules created, shutdown the AppDomains, and continue running. The Bridge will reacquire resources the next time it receives requests.

I think it is fairly critical that after a run all DLL's loaded from the BridgeResourceFolder be released, otherwise the build will fail because it cannot write the new DLL's into the BridgeResourceFolder. I think asking the developer to do an explicit 'reset' is just going to lead to confusing build failures.

@iamjasonp
Copy link
Member

I don't see any issues with this at a glance, so LGTM.
Might be worth merging so that we can see the effects of it running live. When merged, please also keep an eye on the [OuterLoop] for unexpected failures.

roncain added a commit that referenced this pull request Sep 16, 2015
Set BridgeResourceFolder at Bridge startup
@roncain roncain merged commit 08bfbf9 into dotnet:master Sep 16, 2015
@roncain roncain deleted the bridge-config branch September 16, 2015 15: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.

3 participants