⚠️ always use up-to-date context #310
Conversation
| @@ -41,6 +42,9 @@ const javaProjectPom = `<?xml version="1.0" encoding="UTF-8"?> | |||
| // creates new java project and puts the java files in the tree of the project | |||
| // returns path to exploded archive, path to java project, and an error when encountered | |||
| func decompileJava(ctx context.Context, log logr.Logger, archivePath string) (explodedPath, projectPath string, err error) { | |||
| ctx, span := tracing.StartNewSpan(ctx, "decompile") | |||
|
@shawn-hurley there is only one root context we initialize in main.go. All other contexts are derived from it eventually somewhere in the entire call tree, so a cancelled root context will correctly pass down to every child...Is there any other reason to use different contexts in service initialization |
|
Could there be a time when the engine is run multiple times? I think that is the use case? |
|
@shawn-hurley I don't think I completely understand, as long as we pass the same context to engine and all providers here analyzer-lsp/cmd/analyzer/main.go Line 164 in 6ad9f5e analyzer-lsp/cmd/analyzer/main.go Line 136 in 6ad9f5e |
|
Yeah, but could there be a time when the engine is running on its own, and rules come in, and the providers are instantiated when the rules come in (if not set)? Stoping one call to then engine then is different than stopping the full engine. You are not wrong that right now, this will work exactly as you expect, I think keeping them separate prevents some hard debugging in future IMO If we don't think that this is ever a concern that it is not a huge deal. |
|
@shawn-hurley Okay, I understand now. Thanks for explanation. I think your concern is valid, in that case, I think we should split our root context into two different contexts, one that goes into the init call, and another that goes to engine to use in evaluate() calls for each incoming rule. We can do that in future when needed even with this change. |
|
makes sense, then I am good with this change |
|
@pranavgaikwad can you rebase? |
pranavgaikwad
force-pushed
the
fixContext
branch
from
9bce824
to
5a1c361
Compare
|
@shawn-hurley rebased. |
pranavgaikwad
force-pushed
the
fixContext
branch
from
5a1c361
to
4aa72d7
Compare
|
@shawn-hurley Fixed now: added a step in the CI to replace dep |
pranavgaikwad
force-pushed
the
fixContext
branch
2 times, most recently
from
381f538
to
0142612
Compare
|
Sorry need another rebase, I am sorry I tried to reduce this |
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
pranavgaikwad
force-pushed
the
fixContext
branch
from
0142612
to
89aa9f4
Compare
We were using stale context. I observed it when looking at tracer output, some spans were being recorded in the scope of main context, instead of their provider specific context.