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

adapt runner and session for concurrency #320

Closed
plastikfan opened this issue Sep 5, 2023 · 3 comments · Fixed by #325
Closed

adapt runner and session for concurrency #320

plastikfan opened this issue Sep 5, 2023 · 3 comments · Fixed by #325
Assignees
Labels
refactor Refactor code

Comments

@plastikfan
Copy link
Contributor

plastikfan commented Sep 5, 2023

The current implementation of the session and the runner do not play to well when we try to introduce concurrency. This is not because of concurrency per say, but rather we are adding another dimension of functionality requiring an excessive number of changes.

We have 2 types of sessions, primary and resume. The runner drives the session. Either of these session types used to have only a single way of driving the navigator, ie in a synchronous fashion. Then along came concurrency and that was shoe horned in, but not so well. To support concurrency, the following was added:

  • accelerator: added as a property of the runner
  • runner operators: sets concurrency options the accelerator
  • AsyncInfo: contains properties relating to concurrency, unfortunately also containing the context which as I recently discovered should not be stored in a struct because that would lead to the lifetime of the context potentially outliving the go routine it relates to and could be interacted with when it is no longer valid. Actually, it was this issue that led to the need for this refactor requirement.

AsyncInfo has to be passed in as variadic parameter via the following routes:

  • PrimarySession.Run to navigator.ensync
  • ResumeSession.Run to navigator.ensync and then ResumeController.Continue

⚠️ Actually, just writing this out has me thinking, for the resume scenario, why do we have 2 opportunities to pass in AsyncInfo? We should try and remove the one from Continue. In fact, it looks like this was a left over parameter, it is set on strategy.ResumeInfo, which is passed into strategy.resume, but it is not retrieved from here. So let's delete it from Continue.

To fix this, the accelerator needs to be accessed via some other abstraction that has non currency equivalent.

Todo items:

  • Define an extractor specially for the async parameters that receives the params from a slice of any and returns the typed parameters. Let’s call this extractSyncParams
  • By default the sync object will be inline
  • Define a new async operator that by default sets no of workers to NumCPU. This sets the sync object to accelerated. This operator takes no of workers as an optional parameter
  • We now don’t need WithPool operators 
  • Introduce no of workers type def

See the bridge pattern.

We also have to re-consider context. It is not just useful for async scenario. It can and should be used for the inline use case, because we need to support cancellation from ctrl-c interrupt. For this reason, we need to build the context into the inline situation, however we should never need a cancel function that is required by the accelerated scenario. The interrupt can be handled by the client using os.Interrupt

@plastikfan plastikfan added the refactor Refactor code label Sep 5, 2023
@plastikfan plastikfan self-assigned this Sep 5, 2023
@plastikfan
Copy link
Contributor Author

plastikfan commented Sep 28, 2023

The problem with the Run method (on the runner/driver) is that we want a different signature depending on whether the client wants to run inline or accelerated.

  • One way to resolve this is to have the Run method be defined in a way that is compatible with both sync schemes, ie only have path and optionsFn parameters. We then pass the accelerated parameters into Accelerate. The issue with this approach is that we need to be able to define the context on a struct, which we are trying to avoid. But we can appease this demand by implementing a release mechanism invoked as part of the walk (just before it), that passes the temporarily stored context into walk, then discards it immediately. This will ensure that it is not accidentally referenced outside of its valid lifetime.
  • Another way to solve this issue is to pass in a func to Run... The client can pass a function that sets all the appropriate properties, but this still has the downfall of requiring storing the context.
  • But a better way to implement this that does not require storing context on a struct and that is to define Run that take variadic args list of any. There then needs to be an extractor function that can turn these any args into the required args. The downfall of this approach is we lose type safety. The only we can do to compensate for this is to make sure that we panic if we detect incorrect specification of args. The only 3 valid types of args are: context, cancel and *ai.

@plastikfan
Copy link
Contributor Author

plastikfan commented Sep 28, 2023

Some assistance from draw.io is required to get a cohesive design that works across primary/resume and inline/accelerated.

@plastikfan
Copy link
Contributor Author

oops, I discovered a very bad faux par in the naming of the resumeNavigator. It is the front end of the navigator that is used by a primary session and a resume session. There appears to be nothing in it that is related to resume so it should not be called the resumeNavigator; that is very misleading.

plastikfan added a commit that referenced this issue Oct 6, 2023
ref(boost); update lint config for go 1.21 (#320)

ref(nav): type renames (#320)

ref(nav): add runner skelton (#320)

ref(nav): add sync skelton (#320)

ref(nav): partially define sync run (#320)

ref(nav): add context to ensync (#320)

ref(nav): migrate tests to use nav.New (#320)
plastikfan added a commit that referenced this issue Oct 6, 2023
ref(boost); update lint config for go 1.21 (#320)

ref(nav): type renames (#320)

ref(nav): add runner skelton (#320)

ref(nav): add sync skelton (#320)

ref(nav): partially define sync run (#320)

ref(nav): add context to ensync (#320)

ref(nav): migrate tests to use nav.New (#320)
@plastikfan plastikfan linked a pull request Oct 6, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactor code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant