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

Missing exception handling for opening of the resource in AbstractManagedResource.acquireFo #45

Open
naree opened this issue May 6, 2016 · 4 comments

Comments

@naree
Copy link

naree commented May 6, 2016

Hello,

I noticed that AbstractManagedResource.acquireFor is missing exception handing for opening of the resource.

https://github.com/jsuereth/scala-arm/blob/master/src/main/scala/resource/AbstractManagedResource.scala#L87

If this is a bug I am happy to create a fix and create a pull request. But I just wanted to check if there was a reason for not handling exception for opening the resource at this point.

Cheers,
Naree

@jsuereth
Copy link
Owner

Thanks for the question!

Yeah, we are explicitly not handling that exception. If opening the resource fails, that should be handled by an outer-scope. Now, when nesting resources, the "outer" resource will catch exceptions from the "inner" resource and make sure it closes itself.

I could be persuaded to change it so we handle open-failures similarly to close failures, if you have a good argument.

@naree
Copy link
Author

naree commented May 15, 2016

Hi Josh,

Thanks for the reply. Let me try to explain the problem I have with an example.

I like that scala-arm provides monadic operations. I can write code that creates a monadic composition which describes the effect.

  val statement = for {
    connection <- managed(DriverManager.getConnection(jdbcConfig.url, jdbcConfig.user, jdbcConfig.password))
    statement <- managed(connection.createStatement())
  } yield statement

and then execute it when ready without worrying about exception handling and closing resources.

  statement.acquireAndGet { statement => { statement.executeQuery(sql) } }

However, as scala-arm does not handle open failures, in reality the code becomes like this:

  try { 
    statement.acquireAndGet { statement => { statement.executeQuery(sql) } }
  } catch {
    case e: Exception => Left(DBExecutionFailure(e.toMessage())
  }

As you can see the exception handling is leaked outside scala-arm library and side-effect is no longer contained within the monadic composition. If there is a way to solve it without introducing open-failures handling, I will be happy to use it.

Thanks!
Naree

@jsuereth
Copy link
Owner

@naree No, it's more of a design decision on my part than anything else. I think I use the fact that open-throws-exception to shorten a couple coding paths. I'l treat this as a bug and try to fix for 2.0.

Sorry it took me a while to think through the issue. Can you give me a test case for the issue so I make sure it's resolved? I'll implement the back-end code.

@andyglow
Copy link

any progress here, guys?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants