Refactor DispatcherEnv our of the Reactor #864
Conversation
This hides the details of the dispatcher away from the reactor and keeps the code more focused
| { cancelReqsTVar = cancelTVar | ||
| , wipReqsTVar = wipTVar | ||
| , docVersionTVar = versionTVar | ||
| } |
There was a problem hiding this comment.
Getting rid of this is great
|
@alanz are you ok with merging this? |
|
I would like to take a proper look, when I can concentrate. It may be a few days. |
|
@lorenzo is this up to date? I want to start looking at it. |
|
Yes, it is up to date, a few commits behind, but nothing relevant in master that would affect this @alanz |
| else do | ||
| pin <- atomically newTChan | ||
| lspStdioTransport (dispatcherP pin plugins' ghcModOptions) pin origDir plugins' (optCaptureFile opts) | ||
| scheduler <- newScheduler plugins' ghcModOptions |
There was a problem hiding this comment.
This line could be moved out of the if statement, it is in both legs.
There was a problem hiding this comment.
I'm not sure this is possible since scheduler is parameterised for a different base monad on each leg, so the type of scheduler can only be determined after choosing what leg of the if to follow
| liftIO $ Scheduler.sendRequest sc (Just (uri, ver)) req | ||
|
|
||
| -- | Marks a s requests as cencelled by its LspId | ||
| cancelRequest :: (MonadIO m, MonadReader REnv m) => J.LspId -> m () |
There was a problem hiding this comment.
I would expect these functions (all the ones that do asks scheduler to move to the Scheduler module.
Would that make sense?
There was a problem hiding this comment.
something like would make sense to you?
-- In Scheduler.hs
class Monad m => HasScheduler m where
getScheduler :: m Scheduler
-- In Reactor.hs
instance Monad m => HasScheduler (MonadReader REnv m) where
getScheduler = asks scheduler
-- In Scheduler.hs
cancelRequest :: (MonadIO m, HasScheduler m) => J.LspId -> m ()
cancelRequest lId = do
scheduler <- getScheduler
...There was a problem hiding this comment.
Yes, but I suspect the only constraint we really need is that the asks call succeeds.
To me it is about putting all the stuff related to scheduling in one place, so we can look at it holistically.
There was a problem hiding this comment.
How would that look?
There was a problem hiding this comment.
And looking more closely, that is exactly what you proposed. Sorry.
There was a problem hiding this comment.
@alanz I moved the implementation of those functions to the Scheduler module. Decided to keep the same functions in the Reactor module in order to provide some more specific type signatures. Using the generic function from Scheduler in LspStdio required adding many annoying type hints. In my opinion this version looks cleaner.
| -- | Marks a s requests as cencelled by its LspId | ||
| cancelRequest :: (MonadIO m, MonadReader REnv m) => J.LspId -> m () | ||
| cancelRequest lid = | ||
| liftIO . flip Scheduler.cancelRequest lid =<< asks scheduler |
There was a problem hiding this comment.
This was the only function that I could not generalise, I kept getting type errors in the Scheduler module.
|
LGTM, thanks. I reckon we can merge when the CI passes |
This reverts commit 339224f.
|
I'll merge now, tests passed for circle-ci |
This introduces a new
Schedulermodule that that takes much of the code previously in the Dispatcher and removes the channel interface between the reactor and the dispatcher. Instead, explicitly calls to the Scheduler are made to queue requests.closes #835