Move app initialization code to a weaver.Main method. #359
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, application startup code was wrapped up in a function passed to weaver.Run. However this made it hard to test startup code. We now have a `Main` method in `weaver.Main` and place the startup code in the implementation of this method. Refactored startup the internal API used by weavertest to separate out starting the application from running its startup code.
mwhittaker
approved these changes
May 30, 2023
component.go
Outdated
| // This component is instantiated and its Main() method called by | ||
| // `weaver.Run`. | ||
| type Main interface { | ||
| // TODO(reviewer): Should we make this method optional? |
There was a problem hiding this comment.
If someone opts to not implement the Main method, what would be the behavior of weaver.Run? I guess we would still run the main component's Init method and then block forever?
I like making weaver.Ref[weaver.Main] not compile somehow.
There was a problem hiding this comment.
Yes, my intention was that it would block.
I will try to make weaver.Ref[weaver.Main] not compile. And perhaps make the code generator refuse to make RPCs for Main?
| @@ -40,13 +42,14 @@ const matchNothingRE = "a^" // Regular expression that never matches | |||
| // component level configs. config is allowed to be empty. | |||
| // | |||
| // Future extension: allow options so the user can control collocation/replication/etc. | |||
| func initMultiProcess(ctx context.Context, name string, isBench bool, runner Runner, logWriter func(*protos.LogEntry)) (context.Context, func() error, error) { | |||
| func initMultiProcess(ctx context.Context, t testing.TB, isBench bool, runner Runner, logWriter func(*protos.LogEntry)) (context.Context, func() error, error) { | |||
There was a problem hiding this comment.
If I'm understanding correctly, if a main component exists, every weavertest will automatically run its Main method. I think previously you suggested the idea of not running Main automatically and instead having users manually call it like this:
runner.Run(t, func(m weaver.Main) {
m.Main(context.Background())
})Do you still think that's a good idea?
There was a problem hiding this comment.
That's a good point. I think that I am leaning towards sticking with letting weaver/weavertest call Main.Main (as opposed to the user explicitly calling it) to keep things similar between non-test and test code. Otherwise, the user has to remember that in scenario Main.Main is called automatically and in another they are responsible.
Note also that Main.Main will only run in weavelets that are hosting weaver.Main. So this could just be considered part of the issue of how test code controls replication.
There was a problem hiding this comment.
I was writing some unit tests and ran into two things that are a bit annoying with the new API (assuming there is more than one test). They are both related to Main being run more than once.
- You can't call
http.HandleFunc(or any other function that registers an endpoint with the default mux). When the second test runs, it callsMainagain, andhttp.HandleFuncpanics. - If your server listens on a fixed port, like
localhost:9000, the second test will fail with abind: address already in useif yourMaindoesn't respect the context it receives.
I'm not sure how to fix these issues, but wanted to document them somewhere.
spetrovic77
approved these changes
May 30, 2023
component.go
Outdated
| // This component is instantiated and its Main() method called by | ||
| // `weaver.Run`. | ||
| type Main interface { | ||
| // TODO(reviewer): Should we make this method optional? |
There was a problem hiding this comment.
Making it optional would involve maintaining Main as an empty interface and using reflection to discover if the Main impl implements this method?
That seems less clear to me for the user. I like your alternatives below, however.
There was a problem hiding this comment.
Yes. The code is pretty simple, but the question is whether or not it is more complicated for the user. So I will try to just disable Ref[] and RPCs.
mwhittaker
approved these changes
May 30, 2023
| @@ -40,13 +42,14 @@ const matchNothingRE = "a^" // Regular expression that never matches | |||
| // component level configs. config is allowed to be empty. | |||
| // | |||
| // Future extension: allow options so the user can control collocation/replication/etc. | |||
| func initMultiProcess(ctx context.Context, name string, isBench bool, runner Runner, logWriter func(*protos.LogEntry)) (context.Context, func() error, error) { | |||
| func initMultiProcess(ctx context.Context, t testing.TB, isBench bool, runner Runner, logWriter func(*protos.LogEntry)) (context.Context, func() error, error) { | |||
spetrovic77
approved these changes
May 30, 2023
mwhittaker
added a commit
that referenced
this pull request
Jun 20, 2023
> Overview In #359, we changed weavertest to automatically run the main component's Main method for every test. This was done to allow a developer to test the server they started in the Main method. However, we realized that this approach of automatically running the Main method has some drawbacks. Because the Main method is run for every test, if you do anything in the Main method that cannot be run more than once, the tests will fail. For example, if you register a handler with the default serve mux using `http.HandleFunc`, all tests after the first will panic. After some offline discussion, we decided not to run the Main method automatically. Additionally, we'll allow people to get pointers to component implementations in a test. Then, developers can test the guts of their Main method using existing things like the httptest package. This PR changes weavertest to not run the Main method automatically. In a future CL, I'll allow developers to get pointers to component implementations. > Details - I removed weavertest.GetListenerAddress. It will no longer be needed. - I removed the apps variable from the weavertest package. It was only needed to implement GetListenerAddress. - I removed testMainInterface from the weavertest package. I think this was some stale code lingering around from an earlier implementation of weavertest.
mwhittaker
added a commit
that referenced
this pull request
Jun 20, 2023
This PR reverts #359. In particular, it removes the main component's Main method and changes `weaver.Run` to take a lambda. We made this change for the following reason. The Main method is run by `weaver.Run`, but not by `weavertest.Runner.Test`. We felt this was confusing. With the lambda API, it is much clearer what is run. This change also unifies the `weaver.Run` and `weavertest.Runner.Test` API. Both take a lambda with component arguments. Right now, the signature of `weaver.Run` is a bit gnarly. I think I can simplify it a bit in a future PR to remove PointerToMain at least.
mwhittaker
added a commit
that referenced
this pull request
Jun 21, 2023
> Overview In #359, we changed weavertest to automatically run the main component's Main method for every test. This was done to allow a developer to test the server they started in the Main method. However, we realized that this approach of automatically running the Main method has some drawbacks. Because the Main method is run for every test, if you do anything in the Main method that cannot be run more than once, the tests will fail. For example, if you register a handler with the default serve mux using `http.HandleFunc`, all tests after the first will panic. After some offline discussion, we decided not to run the Main method automatically. Additionally, we'll allow people to get pointers to component implementations in a test. Then, developers can test the guts of their Main method using existing things like the httptest package. This PR changes weavertest to not run the Main method automatically. In a future CL, I'll allow developers to get pointers to component implementations. > Details - I removed weavertest.GetListenerAddress. It will no longer be needed. - I removed the apps variable from the weavertest package. It was only needed to implement GetListenerAddress. - I removed testMainInterface from the weavertest package. I think this was some stale code lingering around from an earlier implementation of weavertest.
mwhittaker
added a commit
that referenced
this pull request
Jun 21, 2023
> Overview In #359, we changed weavertest to automatically run the main component's Main method for every test. This was done to allow a developer to test the server they started in the Main method. However, we realized that this approach of automatically running the Main method has some drawbacks. Because the Main method is run for every test, if you do anything in the Main method that cannot be run more than once, the tests will fail. For example, if you register a handler with the default serve mux using `http.HandleFunc`, all tests after the first will panic. After some offline discussion, we decided not to run the Main method automatically. Additionally, we'll allow people to get pointers to component implementations in a test. Then, developers can test the guts of their Main method using existing things like the httptest package. This PR changes weavertest to not run the Main method automatically. In a future CL, I'll allow developers to get pointers to component implementations. > Details - I removed weavertest.GetListenerAddress. It will no longer be needed. - I removed the apps variable from the weavertest package. It was only needed to implement GetListenerAddress. - I removed testMainInterface from the weavertest package. I think this was some stale code lingering around from an earlier implementation of weavertest.
mwhittaker
added a commit
that referenced
this pull request
Jun 21, 2023
This PR reverts #359. In particular, it removes the main component's Main method and changes `weaver.Run` to take a lambda. We made this change for the following reason. The Main method is run by `weaver.Run`, but not by `weavertest.Runner.Test`. We felt this was confusing. With the lambda API, it is much clearer what is run. This change also unifies the `weaver.Run` and `weavertest.Runner.Test` API. Both take a lambda with component arguments. Right now, the signature of `weaver.Run` is a bit gnarly. I think I can simplify it a bit in a future PR to remove PointerToMain at least.
mwhittaker
added a commit
that referenced
this pull request
Jun 21, 2023
This PR reverts #359. In particular, it removes the main component's Main method and changes `weaver.Run` to take a lambda. We made this change for the following reason. The Main method is run by `weaver.Run`, but not by `weavertest.Runner.Test`. We felt this was confusing. With the lambda API, it is much clearer what is run. This change also unifies the `weaver.Run` and `weavertest.Runner.Test` API. Both take a lambda with component arguments. Right now, the signature of `weaver.Run` is a bit gnarly. I think I can simplify it a bit in a future PR to remove PointerToMain at least.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, application startup code was wrapped up in a function passed to weaver.Run. However this made it hard to test startup code. We now have a
Mainmethod inweaver.Mainand place the startup code in the implementation of this method.Refactored startup the internal API used by weavertest to separate out starting the application from running its startup code.