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

improvement: queue or cancel previous connect request #6691

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kasiaMarek
Copy link
Contributor

No description provided.

@kasiaMarek kasiaMarek force-pushed the cancel-build-server-connection branch 4 times, most recently from b1564af to 3f2a29b Compare August 20, 2024 14:51
@@ -211,18 +212,12 @@ class SbtBloopLspSuite
_ = assertNoDiff(
client.beginProgressMessages,
List(
progressMessage,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't deduplicate like this anymore. Instead requests will be queued and the first one will get cancelled. So sbt bloopInstall will be executed twice, and the first request will be cancelled after that, so import and indexing are executed only once.

@kasiaMarek kasiaMarek marked this pull request as ready for review August 21, 2024 08:26
val iter = queue.iterator()
while (iter.hasNext()) {
val curr = iter.next()
request.cancelCompare(iter.next().request) match {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain the logic a bit better here? Could we use some case objects instead of just numbers?


class Interruptable[+T] private (
futureIn: Future[T],
cancelPromise: Promise[Unit],
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we able to forward the cancel request to an underlying process if for example bloopInstall is running?

@kasiaMarek kasiaMarek force-pushed the cancel-build-server-connection branch from 3f2a29b to 044cc69 Compare December 12, 2024 11:31
@kasiaMarek kasiaMarek marked this pull request as draft December 12, 2024 14:45
@kasiaMarek kasiaMarek requested a review from tgodzik January 7, 2025 10:34
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Looks good overall! I like how clear the code is. Is it ready to stop being a draft?

@kasiaMarek kasiaMarek marked this pull request as ready for review January 15, 2025 15:56
@kasiaMarek kasiaMarek force-pushed the cancel-build-server-connection branch from 044cc69 to 22a9bb1 Compare February 3, 2025 09:25
@kasiaMarek kasiaMarek requested a review from tgodzik February 12, 2025 08:47
case class Index(check: () => Unit) extends ConnectRequest {
def cancelCompare(other: ConnectRequest): ConflictBehaviour =
other match {
case _: Disconnect => Queue
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have a Disconnect then we should probably Yield, no? There is no sense to run idnexing after disconnecting?

def cancelCompare(other: ConnectRequest): ConflictBehaviour =
other match {
case (_: Index) | (_: ImportBuildAndIndex) => TakeOver
case _: Disconnect => Queue
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here if we have Disconnect, we should probably just cancel everything and not lose time on import.

@kasiaMarek kasiaMarek requested a review from tgodzik March 11, 2025 14:51
@tgodzik
Copy link
Contributor

tgodzik commented Mar 11, 2025

I don't see any changes since last time, did you forget to push them or my comments were not valid?

@kasiaMarek
Copy link
Contributor Author

I don't see any changes since last time, did you forget to push them or my comments were not valid?

Sorry, my bad.

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

2 participants