GNOME Bugzilla – Bug 769331
Fix throttling in core and lua-factory
Last modified: 2016-09-11 09:15:29 UTC
core had a bug due lack of initialization; lua-factory must re-use the GrlNetWc in order to use throttling. Seems interesting for Bug 741230
Created attachment 332404 [details] [review] net: Initialize GTimeVal for throttling The last_request variable was never initialized leading to delay between request to not work.
Created attachment 332405 [details] [review] lua-factory: Keep GrlNetWc saved as property In order to make usage of throttling from GrlNetWc we need to keep it stored to be used between different operation calls to the lua source. In order to store this pointer, we make usage of source table which is explained in 1074349f2f6ca3edc939313c7ff7d8ff2f5ae53b. The full path for this new table and the GrlNetWc userdata is * grl.__priv_state.properties - grl.__priv_state.properties.net_wc In order to avoid leaks from these properties, the priv_state has now a metattable with __gc set.
Created attachment 332406 [details] [review] lua-factory: One GrlNetWc per lua source Making usage of GrlNetWc property from previous patch.
This needs a test case associated with it. You should be able to start both a server in your test case, and the client in the lua source, so you can make the server assert if the calls are too quick, or really, really too slow (after a timeout). See https://developer.gnome.org/libsoup/stable/libsoup-server-howto.html
(The test case is all the more needed because the patches apparently don't work, as mentioned in IRC)
Created attachment 334082 [details] [review] tests: throttling test on GrlNetWc library
(In reply to Bastien Nocera from comment #5) > (The test case is all the more needed because the patches apparently don't > work, as mentioned in IRC) As far as I recall, the patched worked for Khan but the issue on MusicBrainz webservice was not completely solved. Still, it is always good to have more unit tests :)
Review of attachment 332404 [details] [review]: ::: libs/net/grl-net-wc.c @@ +769,3 @@ priv->last_request.tv_sec += priv->throttling; + + GRL_DEBUG ("delaying web request in %lu", Add a unit to the debug message. Also, what is this representing? The number of seconds until we try the request for real? In which case it should read "delaying web request, retrying in %lu sec"
Review of attachment 332405 [details] [review]: ::: src/lua-factory/grl-lua-library-operations.c @@ +551,3 @@ + priv_state_get_rw_table (L, LUA_SOURCE_PROPERTIES); + lua_getfield (L, -1, prop_name); + /* FIXME: Should we consider all properties as userdata? */ Can you file a bug about this FIXME, and reference it, if it's relevant.
Review of attachment 332406 [details] [review]: Sure.
Review of attachment 334082 [details] [review]: ::: tests/lib-net.c @@ +43,3 @@ +#define NO_DELAY 0 +#define DELAY 2 +#define BIG_DELAY 200 Will it really take 200 seconds to run that test? Seems awfully long. even 10 or 15 seconds would be long enough. @@ +122,3 @@ + +static ThrottlingOperation * +throttling_operation_new(Fixture *f, guint delay_in_sec) space before the paren.
(In reply to Bastien Nocera from comment #11) > Review of attachment 334082 [details] [review] [review]: > > ::: tests/lib-net.c > @@ +43,3 @@ > +#define NO_DELAY 0 > +#define DELAY 2 > +#define BIG_DELAY 200 > > Will it really take 200 seconds to run that test? > Seems awfully long. even 10 or 15 seconds would be long enough. No, the big delay is there to see if the request does not happen before the test timeout, which is 5 seconds. Test is a success as timeout is expected. > > @@ +122,3 @@ > + > +static ThrottlingOperation * > +throttling_operation_new(Fixture *f, guint delay_in_sec) > > space before the paren. Thanks, I'll fix it == == (In reply to Bastien Nocera from comment #9) > Review of attachment 332405 [details] [review] [review]: > > ::: src/lua-factory/grl-lua-library-operations.c > @@ +551,3 @@ > + priv_state_get_rw_table (L, LUA_SOURCE_PROPERTIES); > + lua_getfield (L, -1, prop_name); > + /* FIXME: Should we consider all properties as userdata? */ > > Can you file a bug about this FIXME, and reference it, if it's relevant. I put the FIXME also wondering if we would have more properties. From a quick look in the code, we could have GoaObject too, at least (as userdata too). I'll file a bug about extending this. == == (In reply to Bastien Nocera from comment #8) > Review of attachment 332404 [details] [review] [review]: > > ::: libs/net/grl-net-wc.c > @@ +769,3 @@ > priv->last_request.tv_sec += priv->throttling; > + > + GRL_DEBUG ("delaying web request in %lu", > > Add a unit to the debug message. Also, what is this representing? > The number of seconds until we try the request for real? In which case it > should read "delaying web request, retrying in %lu sec" Yes, number of seconds we try to do the real fetch (not really a retry). I'll add the unit in the debug. Thanks for the review :)
Attachment 332405 [details] pushed as cdab2e7 - lua-factory: Keep GrlNetWc saved as property Attachment 332406 [details] pushed as ebd9119 - lua-factory: One GrlNetWc per lua source
Created attachment 334725 [details] [review] net: Initialize GTimeVal for throttling The last_request variable was never initialized leading to delay between request to not work. --- diff from previous version * add seconds to debug message
Created attachment 334726 [details] [review] tests: throttling test on GrlNetWc library --- diff from previous version * https://paste.fedoraproject.org/420588/47291799/
Review of attachment 334725 [details] [review]: Looks fine otherwise. ::: libs/net/grl-net-wc.c @@ +769,3 @@ priv->last_request.tv_sec += priv->throttling; + + GRL_DEBUG ("delaying web request in %lu seconds", delaying web request *by* XX seconds. delaying *in* would mean you would start the delay in XX seconds (meaning the delay + another delay).
Review of attachment 334726 [details] [review]: > throttling test on GrlNetWc library Add throttling test for GrlNetWc ::: tests/Makefile.am @@ +24,3 @@ registry_SOURCES = registry.c registry_LDADD = $(progs_ldadd) +lib_net_SOURCES = lib-net.c Only if grl-net is built (it's optional). No need to take special care of EXTRA_DIST, grl-net is forced on during the dist phase. ::: tests/lib-net.c @@ +43,3 @@ +#define NO_DELAY 0 +#define DELAY 2 +#define BIG_DELAY 15 // Big enough for a timeout Please use C style comments, not C++.
(In reply to Bastien Nocera from comment #16) > Review of attachment 334725 [details] [review] [review]: > > Looks fine otherwise. > > ::: libs/net/grl-net-wc.c > @@ +769,3 @@ > priv->last_request.tv_sec += priv->throttling; > + > + GRL_DEBUG ("delaying web request in %lu seconds", > > delaying web request *by* XX seconds. > > delaying *in* would mean you would start the delay in XX seconds (meaning > the delay + another delay). ah! got it, thanks! I'll be fixing and pushing this one. (In reply to Bastien Nocera from comment #17) > Review of attachment 334726 [details] [review] [review]: > > > throttling test on GrlNetWc library > > Add throttling test for GrlNetWc > > ::: tests/Makefile.am > @@ +24,3 @@ > registry_SOURCES = registry.c > registry_LDADD = $(progs_ldadd) > +lib_net_SOURCES = lib-net.c > > Only if grl-net is built (it's optional). Ah, true. > > No need to take special care of EXTRA_DIST, grl-net is forced on during the > dist phase. > > ::: tests/lib-net.c > @@ +43,3 @@ > +#define NO_DELAY 0 > +#define DELAY 2 > +#define BIG_DELAY 15 // Big enough for a timeout > > Please use C style comments, not C++. Sure. I'll change it and submit a new one soon. Thanks for the reviews
Comment on attachment 334725 [details] [review] net: Initialize GTimeVal for throttling Attachment 334725 [details] pushed as b4dca11 - net: Initialize GTimeVal for throttling
Created attachment 334802 [details] [review] tests: throttling test on GrlNetWc library --- * changes from previous version - lib-net test should only be built if net library is. - using /* */ comment instead of //
Review of attachment 334802 [details] [review]: Sure.
Attachment 334802 [details] pushed as f3ccf55 - tests: throttling test on GrlNetWc library
Seems to break build on i586: [ 166s] lib-net.c: In function 'soup_server_throttling_cb': [ 166s] lib-net.c:89:41: error: format '%lu' expects argument of type 'long unsigned int', but argument 2 has type 'gint64 {aka long long int}' [-Werror=format=]
Created attachment 335271 [details] [review] build: fix build due wrong format in debug Introduced in commit b4dca1129989f9d9fc4 Reported-by: Dominique Leuenberger <dominique-gnomezilla@leuenberger.net>
(In reply to Dominique Leuenberger from comment #23) > Seems to break build on i586: Oh well, and release was done a few hours ago. > [ 166s] lib-net.c: In function 'soup_server_throttling_cb': > [ 166s] lib-net.c:89:41: error: format '%lu' expects argument of type 'long > unsigned int', but argument 2 has type 'gint64 {aka long long int}' > [-Werror=format=] Could you check if atachment 335271 fixes the issue for you?
(In reply to Victor Toso from comment #24) > Created attachment 335271 [details] [review] [review] > build: fix build due wrong format in debug > > Introduced in commit b4dca1129989f9d9fc4 > > Reported-by: Dominique Leuenberger <dominique-gnomezilla@leuenberger.net> We'll go ahead and test our package right away with this fix (will give feedback shortly) Can you please use my 'dimstar@opensuse.org' address as attribution here?
(In reply to Dominique Leuenberger from comment #26) > (In reply to Victor Toso from comment #24) > > Created attachment 335271 [details] [review] [review] [review] > > build: fix build due wrong format in debug > > > > Introduced in commit b4dca1129989f9d9fc4 > > > > Reported-by: Dominique Leuenberger <dominique-gnomezilla@leuenberger.net> > > We'll go ahead and test our package right away with this fix (will give > feedback shortly) > Can you please use my 'dimstar@opensuse.org' address as attribution here? Sure, fixing it locally
Created attachment 335272 [details] [review] build: fix build due wrong format in debug Introduced in commit b4dca1129989f9d9fc4 Reported-by: Dominique Leuenberger <dimstar@opensuse.org>
That one issue is fixed, just to find the next one : [ 97s] grl-net-wc.c: In function 'get_url': [ 97s] grl-net-wc.c:771:16: error: format '%lli' expects argument of type 'long long int', but argument 5 has type 'glong {aka long int}' [-Werror=format=] [ 97s] GRL_DEBUG ("delaying web request by %" G_GINT64_FORMAT " seconds", [ 97s] ^ [ 97s] ../../src/grl-log.h:127:43: note: in definition of macro 'GRL_LOG' [ 97s] grl_log ((domain), (level), G_STRLOC, __VA_ARGS__); \ [ 97s] ^~~~~~~~~~~ [ 97s] grl-net-wc.c:771:5: note: in expansion of macro 'GRL_DEBUG' [ 97s] GRL_DEBUG ("delaying web request by %" G_GINT64_FORMAT " seconds", introduced in https://git.gnome.org/browse/grilo/commit/?id=b4dca1129989f9d9fc4a274860d38566802cd11c referencing the same bug
(In reply to Dominique Leuenberger from comment #29) > That one issue is fixed, just to find the next one : That comment is wrong - the error is just different - I jumped already one ahead (see below) > [ 97s] grl-net-wc.c: In function 'get_url': > [ 97s] grl-net-wc.c:771:16: error: format '%lli' expects argument of type > 'long long int', but argument 5 has type 'glong {aka long int}' > [-Werror=format=] > [ 97s] GRL_DEBUG ("delaying web request by %" G_GINT64_FORMAT " > seconds", > [ 97s] ^ > [ 97s] ../../src/grl-log.h:127:43: note: in definition of macro 'GRL_LOG' > [ 97s] grl_log ((domain), (level), G_STRLOC, __VA_ARGS__); \ > [ 97s] ^~~~~~~~~~~ > [ 97s] grl-net-wc.c:771:5: note: in expansion of macro 'GRL_DEBUG' > [ 97s] GRL_DEBUG ("delaying web request by %" G_GINT64_FORMAT " > seconds", > > > introduced in > https://git.gnome.org/browse/grilo/commit/ > ?id=b4dca1129989f9d9fc4a274860d38566802cd11c referencing the same bug this error does not show, but I'm rather sure it will, once we get there (I assume it might end up as the same)
From https://developer.gnome.org/glib/stable/glib-Date-and- Time-Functions.html#GTimeVal The values in question seem to be glong (as the compiler also states) so we'd need G_GLONG_FORMAT there it seems (local test running)
Created attachment 335273 [details] [review] Patch fixing the i586 build issue Right idea, wrong file patched in your attempt. The issue is in tests/lib-net.c
Review of attachment 335273 [details] [review]: Looks good.
(In reply to Bastien Nocera from comment #33) > Review of attachment 335273 [details] [review] [review]: > > Looks good. Commit as 3a6268f6