#Could a reader that wasn't closed in another package possibly cause memory leak?

134 messages · Page 1 of 1 (latest)

prisma drift
#

Hello, this is very possibly a very dumb question but also I'm at wits end.

I am using this package: github.com/danielchalef/jsplit to split some very large JSON files (4GB gzipped, ~300 GB decompressed) that is stored on Azure Blob Storage.

When I run the code, I would get this message (not sure if warning or just debug message):

A blob.reader reading from "path/to/file/in/az/blob.json.gz" was never closed (/path/to/danielchalef/jsplit/pkg/cloud/blob.go:69)

Eventually, this uses up all the memory in WSL and crashes it. As in, it would just kill WSL entirely and I have to restart it.

Is it possible that this is the cause of my issue? Or should I look somewhere else?

My Go-fu isn't very good, so I couldn't really figure this out on my own.

Could someone shed some light on this? Very much appreciated.

fading stream
#

300Gb doesn't fit in memory, is probably the issue?

prisma drift
#

I mean I don't think that's the issue. I don't really know how exactly the package does it, but it's supposed to break down the uncompressed 300GB into 4GB chunks/files. It works if I don't store it on the cloud, so it's probably not the JSON data that filling up the memory.

#

The only time this code fails is if I put the source (initial gzipped file) on the cloud.

#

The way jsplit works is if you pass a scheme with the path, it uses gocloud.dev to open that bucket and if you pass just a relative path it just uses path/filepath to read the file.

visual flume
#

are you closing the readers you get back from jsplit.NewReader?

#

the error just shows where the reader was created, but that doesn't mean it's the package's fault—it's still your responsibility to close it

prisma drift
#

Hm, I use jsplit.File() which doesn't return a reader, it just starts the process.

visual flume
#

i don’t see a File() anywhere

prisma drift
#

Sorry, I'm an idiot, I meant jsplit.Split()

fading stream
prisma drift
#

I have the same test file in a folder on my machine and I never run into memory issues. I'm using the same machine to process these test files. The only difference is the where the source is being read from. I get that warning every time I read from blob storage, so I was wondering if that may have anything to do with it.

visual flume
#

yes, i think this is a bug in the library

#

there isn't a single call to Close on a reader in the entire repo

prisma drift
#

Yeah, I was just looking at the code from where this lib was forked from. It was built with using AsyncReaderFromFile() in mind.

#

Oh, it looks like I can just recreate the Split function on my side though, that way I'll have access to the reader.

visual flume
#

this whole thing seems super over-complicated

#

i don't really see the point of reading the bucket in a separate goroutine from the one where the JSON is processed

#

there's so much indirection everywhere it makes it really hard to understand what's going on lol

prisma drift
prisma drift
visual flume
#

i'm not saying your goal is too complicated, i'm saying the library is

#

they pump data around a bunch of different goroutines for not much benefit from what i can see

prisma drift
#

Ohh, okay. Heh. Yeah, I read that they built it on a whim and haven't actively worked on it since 🤷‍♂️ but now I'm stuck with it because I couldn't really find an alternative 😦

#

If I didn't have this step, I'd have to parse the entire 300GB file myself and I don't think I know enough to be able to do that efficiently.

visual flume
#

yeah imo the manual JSON parsing logic they have here is the only thing of value

prisma drift
#

It's crazy how quickly you picked it up though, it took me hours to kind of understand the code lol.

#

damn, I guess I still can't close it because it returns *jsplit.AsyncReader

#

how do you even actually close something like this?

visual flume
#

from what can i tell you can't, because it keeps the reader value unexported and doesn't provide any code path that hits Close

fading stream
#

probably iobuf.Scanner is all you'd need... split every N lines?

visual flume
#

i'm not sure you see what the library is actually doing

#

it's taking a single large JSON object and splitting it up without fully parsing the large object at any one time

fading stream
#

I haven't looked no, just read "jsonl"

visual flume
#

JSONL comes in the output, not the input

fading stream
#

so... if I have { "300gb" } how does it "split" it?

#

(ie it's not even possible to split a single json object, if it was a single one for instance)

visual flume
#

yes, the package only splits arrays, as noted in its README

prisma drift
#

Say you have a JSON object:

{
  "report_name": "some name",
  "list_of_things": [ .. very large list of things ],
  "some_other_list": [ .. also very large ],
}

This comes to about 4GB gzipped or 300GB uncompressed. The package takes the top level arrays and puts each element in it's own line in jsonl.
And then it groups it in 4GB chunks.

so you get:

{ "report_name": "some name", { single element } }
{ "report_name": "some name", { second element } }
.. etc
#

then the file name is the key of that array, like list_of_things_00.jsonl

visual flume
#

and yeah, it's clear to me that it was built for a specific need and not really abstracted into a library in the best way possible

#

if you're willing to steal the manual JSON parsing logic i think you could make things far, far simpler

#

it is a bit of a shame because that part seems quite useful, it's just surrounded by worse design decisions

prisma drift
#

Which part even is that? lol

visual flume
#

the iterator, the stack, and the first 300 lines of jsplit.go

#

it's how they stream the large JSON document without parsing it all at once

#

but all of the reader logic is…questionable, and including support for cloud URLs seems more apt for a tool than a library

#

they force the interface implementation to return a byte slice, then note that they have GC issues in their README

#

there's a reason that io.Reader takes in a byte slice, rather than returning one

#

i see that from people coming from other languages a lot

#

if you haven't used POSIX read, the calling pattern of io.Reader feels a bit weird, so it seems natural to adapt it to returning a slice

prisma drift
#

oof, this went way over my head heh

visual flume
#

but they don't realize that forcing the implementation to allocate becomes a huge issue

#

TL;DR bad, overcomplicated programming

#

it feels like they came in hearing "ooh Go has goroutines, let's use them" and didn't consider whether they're actually a benefit here

prisma drift
#

if I could just ask one more question, if I cloned and maybe just deployed a copy of this. what do I need to change to close the blob.Reader?

#

it seems that when it goes from NewReader() it gets turned into a io.Reader in AsyncReaderFromReader

visual flume
#

i'd add a close on async_reader.go:90, but you'll need to change the reader they store from an io.Reader to io.ReadCloser and consequently fix some other stuff up

#

the fact that they don't even store io.ReadCloser really proves that they can't be closing it 😆

prisma drift
#

Yeah, I think it's because from gocloud, the type is called blob.Reader but it still has a Close() function? Maybe they overlooked it?

visual flume
#

they're doing the same thing for local files, too

#

with a local file it just doesn't manifest in storing the entire thing in memory at once

#

i suppose that without forking, you could call AsyncReaderFromReader and close it yourself after the split finishes

fading stream
#

not sure closing a reader would change anything (isn’t there only 1 big read thing) and many written chunks

visual flume
#

calling SplitStream, rather than Split

#

you'd be responsible for going out and getting the cloud reader, but that function seems legit enough

prisma drift
#

it has Start, Read, and IsClose lol, I'm trying to figure out if I can just put close somewhere in one of these functions

visual flume
#

you can still keep the reference to the reader you passed in and close it yourself

prisma drift
prisma drift
#

Oh, you said AsyncReaderFromReader, I was using AsuncReaderFromFile

#

I will try this.

fading stream
#

it won’t change anything to memory footprint

prisma drift
#

It won't? I wasn't sure if this was what's causing memory issues really but you're saying closing the reader won't fix it?

visual flume
#

are you splitting more than one file in your program?

prisma drift
#

no, just one at a time

fading stream
#

then it won’t

prisma drift
#

after splitting, I read the chunks though

fading stream
#

given ur program gets killed before that 1 close will be reached

#

well when/where do you consume memory ? which phase? might want to profile on a smaller input to see what’s up

visual flume
#

so clearly they’re getting to a point where the reader is being GC’d

#

but its resources aren’t

prisma drift
#

Sorry, I don't really know how to do that exactly. Which is why I'm only asking if it's possible if this is causing my issues.
My approach to this is more of: fix any errors or warnings and see if that resolves my issues 😓

fading stream
#

a reader has no resources (maybe 1 file and 1 buffer) and it’s only 1 anyway. not to say for correctness you shouldn’t close but it won’t cause this symptom (by definition the footprint if any is fine if it reaches the end)

prisma drift
#

I have started to learn using pprof though but I'm still lacking a lot of knowledge how to effectively use it.

#

I am sort of studying as I go

fading stream
#

so it crashes after the split is complete when you do something else?

prisma drift
#

Yes, after the split. I start reading the chunked output but sometimes it doesn't even get to that.

fading stream
#

can you confirm by… not doing something else 🙂 (os.Exit(0) or comment out the call to the rest), if so no point in blaming or analyzing the jsplit lib?

prisma drift
#

Also, another reason why I focused on this is because this whole thing works if I don't use a blob storage (ie. I just store the files locally).

#

and that was the only warning/error I got

fading stream
#

(unless that pushes right to the edge if somehow the gcloud stuff keeps things in memory/cached… )

#

so it’s more likely about the writes than the read and split but again a memory profile would show

prisma drift
#

I'll try to get a mem profile in here as soon as I can, but honestly I still don't understand a lot of it.

#

For memory profiling, I added something like this:

os.Create("profiles/mem.prof")

data.Parse("azblob://path/to/chunks")

err = pprof.WriteHeapProfile(mf)
if err != nil {
  log.Fatal(...)
}

But this only calls Parse which the logic that reads the already split files.
If I add another one where Split is called, would that be enough?

fading stream
#

at the end is most likely enough

#

if you're sure that the only variable between crashing and not crashing is putting the source in the cloud, (vs writing the destination to the cloud) I can take back the unrelated to read

#

what bufferSize do you use?

#

because readCh: make(chan []byte, 16),
will probably leave 16x that number in use

prisma drift
#

for readCh? I just use that one.

fading stream
#

no func AsyncReaderFromFile(uri string, bufferSize int) (*AsyncReader, error) {

#

what bufferSize

prisma drift
#

Ah, I use the saem as what Split uses. I actually haven't run the code that uses AsyncReaderFromFile because I'm still not able to access the reader.
So I'm currently still using the Split

#

I think the value being used there is 1MB 1024*1024

#

oh btw, I should be disabling pprof when parsing large files right?

#

or would that not matter?

fading stream
#

hmm so that'd just be 16Mb ... maybe they keep append'() in somewhere and keeping some huge underlying buffer alive

#

I have pprof as a build tag personally but... probably not necessary to be that paranoid about it...

prisma drift
#

I mean it doesn't balloon in size if I'm processing large data right?

fading stream
#

well as mentioned I would get a profile for a much smaller input, if only to make sure it ends and not crash before getting the profile

prisma drift
#

coz I run this in a WSL instance and if the mem file balloons, the virtual drive also balloons in size and--in true Windows fashion--there is no way to decrease a vh drive once it grows.

fading stream
#

btw, how exactly does it hang/crashes? do you have any message, did you check from another window what top says, or task manager etc

prisma drift
fading stream
#

profile is smaller than you 400gb files

prisma drift
#

We have a very sad kind of setup here, we actually have to work through Azure Virtual Desktop
because they use tracking software to track our "productive hours". So I have to use WSL on a Virtual Windows because the tracking software doesn't work on Linux 😓

#

I'll try to get a mem profile on the weekend, when I can test this on my laptop.

#

If you'll still be here, that is. lol.

fading stream
#

sad indeed 🙂 you should get one of these "move the mouse" stuff 🙂 and then use a real setup for the actual work

prisma drift
#

Apparently that has been tried multiple times and people have been caught 😆

#

I'm pretty new here so I try not to make waves, but I am thinking of talking to our engineering vp about it..

#

coz I've been here like 3 months and I'm already burnt out

fading stream
#

productivity should be measured by output/results, not hours (or line of code or any bs metric)

prisma drift
#

In any case, thank you again! I'm gonna try to see if we can just do away with storing this stuff over cloud storage and have them mount a large enough file drive on whatever server this'll be running from =/

fading stream
#

well it's useful to figure out the source but yes that could be a workaround. yw

prisma drift
#

so we're getting slapped with the same policies and rules which don't really make sense for devs especially

prisma drift
#

again thank you! it's 3:45 AM here and I'm almost off my shift.

#

good night/day!

fading stream
#

take care