mruby sleep #1330

i110 · 2017-06-16T15:30:58Z

Add sleep function to the Kernel.
What drove me to implement this is mainly H2O::Redis reconnect loop, but this can be useful for various situations as pointed out in #1283

NOTE: This PR is toward #1173.

kazuho

Thank you for the PR. Left my comments in-line. Please let me know what you think.

kazuho · 2017-06-20T07:17:31Z

lib/handler/mruby/sleep.c

+    } else {
+        *run_again = 1;
+        return mrb_exc_new_str_lit(mrb, E_ARGUMENT_ERROR, "argument must be either Fixnum or Float");
+    }


I believe that ruby methods are expected to convert the input to appropriate type rather than raising an error upon seeing an unexpected type.

CRuby converts the argument if and only if it responds to divmod (see: https://github.com/ruby/ruby/blob/trunk/time.c#L2266), so for example, if we pass a String to sleep function, it raises a TypeError without any conversion.

Then, regarding the compability with CRuby, it may be better to convert the argument using divmod like CRuby does, but the conversion logic is a bit complicated, and I guess that it's very rare case to pass custom object to sleep function.

So, at this time, I feel restricting the argument type is a reasonable choice.

Thank you for the clarification. I did not know that the sleep method of cruby depends on the existence of divmod.

OTOH, I still believe that we should follow the principle of duck typing rather than checking the types. Can we simply call to_f and then use the returned value instead of doing a switch-case?

kazuho · 2017-06-20T07:18:48Z

lib/handler/mruby/sleep.c

+    ctx->ctx = mctx;
+    ctx->receiver = receiver;
+    h2o_timeout_init(ctx->ctx->shared->ctx->loop, &ctx->timeout, msec);
+    ctx->timeout_entry = (h2o_timeout_entry_t){0};


Is there any reason to zero-clear ctx->timeout_entry considering that we are doing memset?

kazuho · 2017-06-20T07:42:21Z

lib/handler/mruby/sleep.c

+
+    /* defer freeing to avoid concurrent modification onf timeout linklist */
+    entry->cb = on_deferred_timeout;
+    h2o_timeout_link(shared->ctx->loop, &shared->ctx->zero_timeout, entry);


Why do you need to defer the disposal of h2o_timeout_t?

Is it due to how h2o_timeout_run is implemented? If that is the case, I think that we should address the issue by fixing h2o_timeout_run (though if that seems like difficult, I am not against doing that after merging this PR as-is).

The reason we have to defer is rather in h2o_evloop_run.

If we dispose h2o_timeout_t in timeout callback function, the linklist enumaration doesn't work anymore (because node = node->next causes badaccess error)

Do you have any idea to address this issue?

I think that we should resolve the issue after merging the PR using the current approach.

Basically what you need to do is record the address of h2o_timeout_t that is being processed by h2o_evloop_run as a member of h2o_evloop_t, and if the address matches that of the disposed timeout, then update the recorded address so that it would point to the previous object. However implementing this correctly might require some work.

I mostly understand the approach you said. OTOH, I want to clarify the disadvantage of the current approach. Are you worried about the effect to the performance?

I wouldn't be worried about the impact of retaining one pointer per every call to h2o_timeout_run and referring to the value in every call to h2o_timeout_dispose, since we call h2o_timeout_run only a few times per once a event loop, and since it is rare to call h2o_timeout_dispose.

If the cost of either of the two becomes an issue, I think that we need to change the architecture of how we maintain the structures, such by changing it from a linked list to a red-black tree ordered by when the timeout events to be triggered, etc.

Anyways, I'll merge this PR as-is. I do not want to delay the introduction of sleep by discussing how we should refactor our timeout design.

i110 · 2017-06-20T09:43:11Z

@kazuho Thank you for your comments. I addressed one and replied to the others.

i110 · 2017-06-21T14:34:34Z

@kazuho I fixed a issue and added tests (I forgot to git add)

kazuho

If performance is a concern, you could do something like:

if (type_is_int) {
    fetch_int;
} else {
    if (!type_is_float) {
        invoke(`to_f`)
        if (!type_is_float)
            raise_error;
    }
    fetch_float;
}

kazuho · 2017-06-22T03:22:17Z

lib/handler/mruby/sleep.c

+    if (! mrb_respond_to(mrb, arg_sec, mrb_intern_lit(mrb, "to_f"))) {
+        *run_again = 1;
+        return mrb_exc_new_str_lit(mrb, E_ARGUMENT_ERROR, "the argument of the sleep function must respond to 'to_f' method");
+    }


Is it necessary to call mrb_respond_to? I thought that mrb_funcall will return an method invocation failure (that we can propagate to the caller) in case the object fails to respond to the method being invoked.

@kazuho Yes, but I thought the users never know that the h2o core calls to_f internally, so
the error message might be very confusing. It's better to show some good error message.

Well, I would argue that you can spend time me to emit an error message that would satisfy the needs of any programmer in case a error occurrs, and doing so will not hurt the performance of the successful path.

I don't get your point. I just wanted to show kind error message to the users, and I'm not worried about performance here.

without respond_to check:

[h2o_mruby] in request:/:mruby raised: NoMethodError: undefined method 'to_f' for #<Object:0x102831620>

with the check:

[h2o_mruby] in request:/:mruby raised: (eval):10: the argument of the sleep function must respond to 'to_f' method (ArgumentError)

I feel the latter is more like an error message that would satisfy the needs of any programmer you said. isn't it?

My point is that if you want to emit a friendly error, you can do the following:

call to_f

if the invocation ended up in an exception, check if the error is a NoMethodError and then adjust the message of the exception

ah I understand. okok!

@kazuho fixed it at f54bfda

add mruby sleep

cabda0b

kazuho reviewed Jun 20, 2017

View reviewed changes

remove needless zero-clear

66793de

i110 added 2 commits June 21, 2017 23:32

simply call to_f

071b4b8

add the test for mruby-sleep

cf77c11

kazuho reviewed Jun 22, 2017

View reviewed changes

remove respond_to call

f54bfda

kazuho merged commit c23773e into i110/mruby-consistent-fiber-handling Jun 26, 2017

kazuho mentioned this pull request Jun 26, 2017

Refactor the timeout design #1340

Open

i110 mentioned this pull request Jun 30, 2017

mruby sleep #1348

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mruby sleep #1330

mruby sleep #1330

i110 commented Jun 16, 2017 •

edited

kazuho left a comment

kazuho Jun 20, 2017

i110 Jun 20, 2017 •

edited

kazuho Jun 21, 2017

kazuho Jun 20, 2017

kazuho Jun 20, 2017

i110 Jun 20, 2017

kazuho Jun 21, 2017

i110 Jun 21, 2017

kazuho Jun 26, 2017

i110 commented Jun 20, 2017

i110 commented Jun 21, 2017

kazuho left a comment

kazuho Jun 22, 2017

i110 Jun 22, 2017

kazuho Jun 22, 2017

i110 Jun 22, 2017

kazuho Jun 23, 2017

i110 Jun 23, 2017

i110 Jun 24, 2017

mruby sleep #1330

mruby sleep #1330

Conversation

i110 commented Jun 16, 2017 • edited

kazuho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i110 Jun 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i110 commented Jun 20, 2017

i110 commented Jun 21, 2017

kazuho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i110 commented Jun 16, 2017 •

edited

i110 Jun 20, 2017 •

edited