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

Normal stream errors unexpectedly kill the fiber in multipartUpload #188

Closed
chriskite opened this issue Mar 23, 2021 · 3 comments · Fixed by #193
Closed

Normal stream errors unexpectedly kill the fiber in multipartUpload #188

chriskite opened this issue Mar 23, 2021 · 3 comments · Fixed by #193

Comments

@chriskite
Copy link

The signature for multipartUpload is:

def multipartUpload[R](
  bucketName: String,
  key: String,
  content: ZStream[R, Throwable, Byte],
  options: MultipartUploadOptions
)(parallelism: Int): ZIO[R, S3Exception, Unit] =

content, the stream passed in by the library user, has the error channel type Throwable, indicating that it is allowed to fail with a Throwable.

However, multipartUpload does the following:

.refineOrDie {
  case e: S3Exception => e
}

Now the Throwable we were allowed to fail with causes an unexpected fiber death. This caused a production issue for us, as without looking into the code we would not have known that something allowed in the method signature would be considered a fiber-killing code defect.

This behavior was introduced in #170 and I would argue it is incorrect. A stream failing with a normal error is not a defect that cannot be recovered from. I would suggest that instead of dying or wrapping content stream errors with a custom type, just keep the Throwable type as the return error channel type for multipartUpload.

I would be happy to put together a PR for this change.

@regis-leray
Copy link
Member

lets revert it ! please create a PR

@chriskite
Copy link
Author

Can do. Do we want to just revert it and go back to the previous approach of wrapping exceptions, or switch to a different approach like returning a Throwable as the error type?

@regis-leray
Copy link
Member

regis-leray commented Mar 27, 2021

@chriskite Lets remove all refineOrDie, thank you

Please change the signature to have

return type ZIO[R, Throwable, Unit]

def multipartUpload[R](
  bucketName: String,
  key: String,
  content: ZStream[R, Throwable, Byte],
  options: MultipartUploadOptions
)(parallelism: Int): ZIO[R, Throwable, Unit] 

Also can you do the changes for getObject

def getObject(bucketName: String, key: String): Stream[Throwable, Byte]

thank you

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 a pull request may close this issue.

2 participants