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

Clarification of return values #51

Open
jhellan opened this issue Apr 1, 2014 · 5 comments
Open

Clarification of return values #51

jhellan opened this issue Apr 1, 2014 · 5 comments
Labels

Comments

@jhellan
Copy link

jhellan commented Apr 1, 2014

Hi

Not strictly an issue, but I did not find an answer in the docs. What are the possible return values from 'save_changes'? I was assuming an array, possibly empty, or an exception. But we have the following lines in our code, where spf_vmm_service is an OData::Service instance:

          @spf_vmm_service.AddToVirtualMachines(vmspec)
          vm = @spf_vmm_service.save_changes[0]

but we occasionally see the error

          undefined method `[]' for true:TrueClass (NoMethodError)

from the second line. It looks like save_changes has returned 'true'. Is that possible, and do you have any idea why?

This is isn't easy for us to reproduce right now - it happened just three times during a 60 hr test run which hit this code abt 1100 times.

Do you have any ideas about this?

Jon Kåre Hellan, UNINETT AS, Trondheim, Norway

@visoft
Copy link
Owner

visoft commented Apr 2, 2014

Hmm, that's a weird one. My only guess at this point would be that the response from the server is mangled somehow. I'd love to see the response, but 3 times out of 1100 it seems unlikely. To make it easier to capture, edit the save_changes method to verify the result is an array, if not, dump the result to a text file or something. Is that something you could try? I can add a check in there and do something similar. E.g. throw a custom exception, and/or log the output, but I'd like to be able to reproduce the "actual conditions" to test the problem/solution. I have no real idea at this point what would cause that to happen.

@visoft visoft added the Question label Apr 2, 2014
@jhellan
Copy link
Author

jhellan commented Apr 3, 2014

Thanks. I've now instrumented our code. We'll see what that turns up.

We noticed that for batch saves and operations besides 'Add', save_changes returns a boolean. We also see that we get this error a fraction of a second after an earlier failure from AddToVirtualMachines. Is it possible that a failed save may leave behind a save
operation in the queue, so that the next save gets turned into a batch save?

We've looked for races in our own code, but the block
{ AddToVirtualMachines; save_changes } is protected by a mutex. I don't see cases where we call Add but don't call save_changes, either.

@jhellan
Copy link
Author

jhellan commented Apr 7, 2014

We managed to get a log which made sense. We checked for non-array in single_save, and for batch saves. Our application doesn't do batch saves intentionally.

We got one save_changes call with op AddToVirtualMachines which resulted in "Server returned error but no message. (OData::ServiceError)". There was no associated http_code.

Turned out that the next save_changes call was indeed performed as a batch_save, which returned true. The @save_operations.length was 2.

Is the save_operations queue supposed to be cleared after the first exceptions? If not, how could we clean it up from the application? Would we have to instantiate a new service object?

@visoft
Copy link
Owner

visoft commented Apr 7, 2014

It doesn't clear the queue after an exception, as you found out. The reasoning here was that if you had 5 changes and one bombed out, you could potentially remove that call. At this point though, there isn't a way to modify the @save_operations array. This should be changed. Perhaps simply exposing that instance variable would suffice, and/or a method to clear out the array.

To reset things, at this point, I believe you'd need to instantiate a new service object like you said.

What would you recommend for a fix? Just expose the array for now?

@jhellan
Copy link
Author

jhellan commented Apr 7, 2014

Hmm. I think it would be cleaner to make single save and batch save two different operations. If a single save fails, it should be cleared, and it will be up to the user to resubmit.

In a batch situation, can the user tell which ops succeeded and which failed? If he can't, there's obviously no point in keeping the queue around. If he can, I guess it would be best to expose the clear queue operation.

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

No branches or pull requests

2 participants