Fork me on GitHub
#aleph
<
2017-09-18
>
ztellman01:09:55

@aengelberg @dm3 as a general rule, if there's an error somewhere in a stream transform or operation, the error will be logged and the stream will be closed

ztellman01:09:16

which should lead to things being cleaned up

ztellman01:09:28

if that's not true in some situations, I'd consider that a bug in my implementation

aengelberg01:09:15

I get that it usually cleans up resources properly, but it does not propagate the fact that something went wrong. A stream closing could either mean success or failure.

aengelberg02:09:45

core.async chans have a similar problem.

ztellman02:09:08

error propagation is a bit tricky in general, imagine if src is being split across two different sinks, a and b

ztellman02:09:22

if a fails, should that cause src to yield an error too?

ztellman02:09:38

or should it just continue to feed into b?

ztellman02:09:52

more to the point, if you're feeding into a network socket and it closes, is that correct behavior or an error?

ztellman02:09:40

I don't try to give anything richer than TCP offers, basically

ztellman02:09:23

in order to talk about something succeeding at the application level, you need to define your success criteria using logic layered atop the transport layer, not in the transport layer

ztellman02:09:03

in fairness, I absolutely could have put! fail with an error rather than return false if, say, map fails

ztellman02:09:37

but I suspect that people aren't checking for that, and in practice it would just cause the same error to be logged all the way up the topology

aengelberg02:09:07

Great points

ztellman02:09:25

this isn't to say that you couldn't create a richer error model, I'm sure other stream libraries do

ztellman02:09:11

but being the lowest common denominator across all stream abstractions makes that difficult

aengelberg02:09:39

My original complaint stemmed from the fact that I wanted to convert InputStream into (seq-of bytes), and byte-streams was (unnecessarily?) using a manifold stream in the conversion path, and that conversion silenced errors such as amazon.s3.SocketTimeoutException that I wanted the consumer of the lazy seq to see.

ztellman02:09:23

it wasn't even logging the error?

aengelberg02:09:27

It was logging the error

aengelberg02:09:29

which is great

ztellman02:09:35

but it wasn't propagating it upwards

ztellman02:09:46

that is an unnecessary conversion to a stream, I think

aengelberg02:09:50

which I need to happen for my job to die and mark itself as failure

ztellman02:09:01

I just need to define a new edge in the conversion graph

ztellman02:09:09

but I think the same issue would happen if the connection just closed, right?

aengelberg02:09:21

it depends on the implementation

ztellman02:09:22

is the S3 client validating the length of the value?

aengelberg02:09:37

in this case I was reading from S3, not writing to it

ztellman02:09:52

right, so what happens if the connection closes halfway through downloading the value

ztellman02:09:56

is an exception thrown?

aengelberg02:09:59

but I have a different, separate use case that also suffered from this problem in which I was uploading each chunk as a batch to S3 which could potentially fail

ztellman02:09:24

because if so, that strikes me as a little weird, you wouldn't expect the InputStream to be doing that for you

aengelberg02:09:26

good question, I'd have to think back to the last time I saw this error happen in prod

ztellman02:09:05

and this comes back to my original point of "you get some data, it's up to you to decide if it's the correct, full data"

ztellman02:09:27

I know you don't control the S3 client api, so I'm not really pointing the finger at you here

ztellman02:09:13

but with anything over the network, lack of an explicit error is not a guarantee that everything's correct, which is kind of why I haven't worried about this part of Manifold very much

aengelberg02:09:28

I could easily reproduce this by just manually opening an inputstream, waiting out the timeout, and seeing what happens

ztellman02:09:30

(which isn't a great defense, tbh, but that's my rationale)

aengelberg02:09:40

so basically my takeaway here is that I need to figure out what exact error I want to be able to see before I complain about not seeing it

ztellman02:09:45

possibly, or at least consider that failing to process everything isn't always signaled by an exception

ztellman02:09:37

so having exception propagation doesn't actually mean all issues are surfaced, and may lead to thinking that things are correct when they aren't

aengelberg02:09:29

I'm fairly confident that my S3 read sockettimeout issue was somehow surfacing through one of the InputStream methods. But I'll need to confirm since you make an interesting point that it might just close it instead