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

Add test case that exhibits portal does not exist problem. #904

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

Conversation

matthughes
Copy link
Contributor

In trying to track down the "portal does not exist" problem #553, I was able to create a test case that demonstrates the issue.

My code had a weird construction of .map(effect).flatMap(identity) instead of flatMap(effect). I was able to reliably reproduce the bug in my code with the former and was never able to reproduce with the latter. I was able to replicate this in Skunk code.

  • flatMap(effect) no issue
  • map(effect).flatMap(identity) inside of resource acquisition, no issue
  • map(effect) with flatMap(identity) outside of resource acquisition, portal not found
  • map(effect) with flatten outside of resource acquisition, portal not found

Comment on lines +137 to +143
pool.use { s =>
s.transaction.use { _ =>
s.prepare(cmd(idx)).flatMap(_.execute((s"name$idx", idx))).map (_ =>
IO.sleep(1.second) >> s.prepare(cmd(idx)).flatMap(_.execute((s"name$idx", idx))).void
)
}
}.flatten
Copy link
Member

Choose a reason for hiding this comment

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

.use(...).flatten looks wrong. It means you are building an IO in the use(...) when the Resource is available, but you are executing it after the use, when the resource has been released.

Comment on lines +122 to +128
pool.use { s =>
s.transaction.use { _ =>
s.prepare(cmd(idx)).flatMap(_.execute((s"name$idx", idx))).map (_ =>
IO.sleep(1.second) >> s.prepare(cmd(idx)).flatMap(_.execute((s"name$idx", idx))).void
)
}
}.flatMap(identity)
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

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