systemincloud / thriftr

Other
17 stars 0 forks source link

Error in private$trans$read(sz) #5

Closed randyzwitch closed 5 years ago

randyzwitch commented 6 years ago

When calling some thrift methods, I do get a valid response. However for some, I just get Error in private$trans$read(sz) : as my error, with no real clue about the problem.

Posting here for now, will try and debug some and come back with either a reproducible example or more information

randyzwitch commented 6 years ago

Pretty sure the issue is here, though trying to dig in further.

https://github.com/systemincloud/thriftr/blob/c82252919ee16d4ba9a8f1b594b81cf619d5b33b/R/protocol_binary.R#L316

marekjagielski commented 6 years ago

Is there no stacktrase or anything more? At the end transport is implemented here: https://github.com/systemincloud/thriftr/blob/c82252919ee16d4ba9a8f1b594b81cf619d5b33b/R/transport_socket.R Probably not the best error handling

read = function(sz) {
      ret <- readBin(self$sock, raw(), sz)
      if (length(ret) == 0) {
        stop()
      }
      ret
}

But maybe you are ending in this stop().

marekjagielski commented 6 years ago

Please add some message to it (worth contribution). If it is here, next question is why you get empty ret.

randyzwitch commented 6 years ago

I'm using the actual R debugger, not doing statements. I'll try to dig into this further, it's pretty hard to follow this generated code TBH :laughing:

randyzwitch commented 6 years ago

I can confirm the problem is here:

read = function(sz) {
      ret <- readBin(self$sock, raw(), sz)
      if (length(ret) == 0) {
        stop()
      }
      ret
}

I commented out the stop() statement, and it caused the function to never return. So that's not a proper fix.

My hypothesis is that the error is ocurring because of empty struct fields. In thriftpy, here's the result of the call that is erroring out in thriftr:

In [7]: client.get_memory(conn, "cpu")                                          
Out[7]: [TNodeMemoryInfo(host_name='', page_size=512, max_num_pages=25692761, num_pages_allocated=0, is_allocation_capped=False, node_memory_data=[])]

You can see that in two places, host_name and node_memory_data there are empty values.

marekjagielski commented 6 years ago

The hangs are because socketConnection is 'blocking' function.

I would check here: https://github.com/systemincloud/thriftr/blob/9fe1d7224cf9aedfe5d7fe4baf9b2f90463887c4/R/protocol_binary.R#L243 if the size 'sz' is 0. If yes, probably we shouldn't do 'inbuf$read(sz)' at all.

marekjagielski commented 6 years ago

The same for LIST: https://github.com/systemincloud/thriftr/blob/9fe1d7224cf9aedfe5d7fe4baf9b2f90463887c4/R/protocol_binary.R#L264

marekjagielski commented 5 years ago

@randyzwitch can you check the newest master version with your example? I added handling for empty strings, lists and maps.

randyzwitch commented 5 years ago

Work is a little crazy right now, but hope to check this out in the next couple of days

randyzwitch commented 5 years ago

These updates look promising! I tried the equivalent R version of client.get_memory(conn, "cpu") that worked in Python and it returned a result in R. I also tried another method and it also returned.

I need to spend a little more time with this, as R6 in general is a bit hard for me to parse, but it does look like you caught the issue.

marekjagielski commented 5 years ago

If you find any more problems with this, please reopen this bug.

randyzwitch commented 5 years ago

Will do, thanks for all your work to fix this

On Thu, Mar 28, 2019 at 4:06 PM Marek Jagielski notifications@github.com wrote:

If you find any more problems with this, please reopen this bug.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/systemincloud/thriftr/issues/5#issuecomment-477751274, or mute the thread https://github.com/notifications/unsubscribe-auth/ACooIxO3Eoaor2hjb9Mtk8q7jhzHA3grks5vbSC4gaJpZM4YM3W_ .